Review what we should call from the Slic read frames loop
> > 7\. I am pretty sure this ReadFrameAsync method executes in the critical "read loop" where we need to be really careful about not doing too much work
I would open another issue for looking into Slic performance improvements.
This is not directly a performance issue but a design issue. Which activities are ok in the "read loop" of Slic/IceProtocolConnection?
Ok and necessary:
- reading from the duplex connection into a PipeWriter
- decoding frame headers
- blocking reads for a while on purpose to put back pressure on the caller
Not acceptable:
- calling "synchronously" application code (since it can be a CPU intensive call that takes a while to complete or yield)
It's not clear to me where sending the Pong frame should be. It should indeed complete quickly (not CPU intensive), but it seems gratuitous to execute this code in the read loop.
See: https://github.com/icerpc/icerpc-csharp/blob/15a2d6d632c7b7dbba9042722c03d312753691da/src/IceRpc/Internal/IceProtocolConnection.cs#L1146
https://github.com/icerpc/icerpc-csharp/blob/15a2d6d632c7b7dbba9042722c03d312753691da/src/IceRpc/Transports/Slic/Internal/SlicConnection.cs#L1023
Originally posted by @bernardnormier in https://github.com/icerpc/icerpc-csharp/issues/3273#issuecomment-1575754712
It should indeed complete quickly (not CPU intensive), but it seems gratuitous to execute this code in the read loop.
See also https://github.com/icerpc/icerpc-csharp/pull/3348#discussion_r1227099748
Should the read frame loop just read the frame body and always schedule a task for executing the code to process the frame? That's the code executed after reading a close frame, a ping or pong frame and all the the stream ReceivedStreamXxxFrame methods executed after decoding a stream control frame.
I find this overkill, these methods don't call any application code and complete quickly.
To some degree, this actually makes our Slic implementation even more vulnerable to DDOS attacks. An attacker can flood the Slic connection with many such Slic frames and since the processing of these frames is scheduled on a separate task, the attacker can more easily flood the connection with other frames to queue more tasks.
We need to better evaluate these vulnerabilities, see https://github.com/icerpc/icerpc-csharp/issues/3317