-
Notifications
You must be signed in to change notification settings - Fork 593
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
Minimal System.IO.Pipelines integration to prepare for full-async work #1264
Conversation
Thanks, I'll review this coming week. |
33baabc
to
cab159f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems reasonable to me and tests pass, so 👍 . I'm going to wait on another review before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look good to me 👍 Tests pass and both test apps run without errors. Interestingly, CreateChannel
test app seems to run 1 second faster (7.6s in main
vs 6.5s) with these changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly late to the party. Sorry!
42b2813
to
8669618
Compare
Thanks @danielmarbach ... as you can see I'm working through your PR review comments. Thanks a MILLION. |
Great to see traction here :) thanks @danielmarbach and @lukebakken. I've been less active lately while I'm transitioning into a new job as well as doing other OSS projects and public speaking, but the RabbitMQ client is always on my list of things to revisit :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍 Let's give other reviewers some time to comment before we merge.
Like your profile pic! I used to work on EVE Online for CCP Games :) |
Make sure read/write tasks for connections run on dedicated threads.
Co-authored-by: Daniel Marbach <daniel.marbach@openplace.net>
Co-authored-by: Daniel Marbach <daniel.marbach@openplace.net>
Co-authored-by: Daniel Marbach <daniel.marbach@openplace.net>
Co-authored-by: Daniel Marbach <daniel.marbach@openplace.net>
Co-authored-by: Daniel Marbach <daniel.marbach@openplace.net>
…e of this method. Copied from the Java client.
…ffer to the array pool
…sting, fix nullable warning
I plan on merging this as soon as CI passes, FYI! |
Thank you @stebet |
Thanks for picking up the remaining work @lukebakken. Hopefully the full-async work can begin soon :) |
Proposed Changes
This introduces
System.IO.Pipelines
into the RabbitMQ Client. I decided to take the minimal step required to add it, as a way to break down PR number #1199 into more manageable parts.This has no API changes, it still has the Channels for transmission buffering (to be changed later), but instead of reading/writing directly to a
BufferedStream
, we are now using Pipelines which handle the buffering and back-pressure.This paves the way forward to slowly get rid of the Channels as well, and to create a fully and proper asynchronous code paths for sending/receiving data in a performance manner.
Types of Changes
Checklist
CONTRIBUTING.md
document