Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
bentoi opened this issue Jun 7, 2023 · 1 comment
Open

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

bentoi opened this issue Jun 7, 2023 · 1 comment
Labels
slic Slic Transport
Milestone

Comments

@bentoi
Copy link
Contributor

bentoi commented Jun 7, 2023

          > > 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:

while (!cancellationToken.IsCancellationRequested)

private Task ReadFrameAsync(FrameType type, int size, ulong? streamId, CancellationToken cancellationToken)

Originally posted by @bernardnormier in #3273 (comment)

@bentoi bentoi mentioned this issue Jun 7, 2023
@bernardnormier bernardnormier added the slic Slic Transport label Jun 7, 2023
@bentoi
Copy link
Contributor Author

bentoi commented Jun 13, 2023

It should indeed complete quickly (not CPU intensive), but it seems gratuitous to execute this code in the read loop.

See also #3348 (comment)

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 #3317

@bernardnormier bernardnormier added this to the 0.2 milestone Jun 13, 2023
@bernardnormier bernardnormier modified the milestones: Future, 0.2.0 Sep 19, 2023
@pepone pepone modified the milestones: 0.2.0, Future Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
slic Slic Transport
Projects
None yet
Development

No branches or pull requests

3 participants