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

Switch to long running tasks for the multiplexer #344

Draft
wants to merge 65 commits into
base: main
Choose a base branch
from

Conversation

bdraco
Copy link
Contributor

@bdraco bdraco commented Feb 9, 2025

Testing TODO:

Known issues

Some considerations

  • The timeout is now enforced for the read/write of the message so if it takes more than timeout to read or write the message it will timeout. Thats probably not a problem since the ping/pongs would be blocked and fail the connction anyways.

While these changes are designed for the client side, this combination of these changes is expected significantly increase the number of active connections a serve can handle at one time.

Instead of creating many small tasks, create two long running ones for the reader/writer to avoid flooding the event loop with tasks when the connection is generating many packets.

This is still a bit of a WIP, but should be deferred until after a release with #303 so they get separate release cycles

@bdraco
Copy link
Contributor Author

bdraco commented Feb 9, 2025

We could probably have a single low res TimerHandle that resets on each loop and has a 10s leeway to re-arm. So we ensure we cancel both read/write tasks within timeout+10s. That we we aren't churning timer handles

EDIT: with the task overhead gone, that is where all the time gets spent

@bdraco
Copy link
Contributor Author

bdraco commented Feb 9, 2025

That test fails locally before and after this change so I think its intent is to overload the queue but its too fast now, and my local machine is too fast as well before the change.

The queue size needs to be patched to be smaller so it can be overrun before it can be processed

@bdraco bdraco changed the title Switch to long runner tasks for the multiplexer DEFER: Switch to long runner tasks for the multiplexer Feb 9, 2025
@bdraco
Copy link
Contributor Author

bdraco commented Feb 9, 2025

Maybe a RangedTimeout that does a callback on timeout
So we can timeout between 90 and 100s and only reschedule timer handles if outside range

@bdraco
Copy link
Contributor Author

bdraco commented Feb 13, 2025

test_multiplexer_data_channel_abort_full previously relied on overloading the channel.. but it can keep up now so it doesn't overload

@bdraco
Copy link
Contributor Author

bdraco commented Feb 25, 2025

This is functional but test_multiplexer_data_channel_abort_full needs to be fixed since it doesn't abort anymore because the performance bottleneck / cpu drain is fixed and it no longer gets so far behind that the failure happens

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant