icerpc / icerpc-csharp

A C# RPC framework built for QUIC, with bidirectional streaming, first-class async/await, and Protobuf support.
https://docs.icerpc.dev
Apache License 2.0
102 stars 13 forks source link

Review what we should call from the Slic read frames loop #3318

Open bentoi opened 1 year ago

bentoi commented 1 year ago
          > > 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:

Not acceptable:

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

bentoi commented 1 year ago

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