Conversation
SergiySW
previously approved these changes
Sep 8, 2020
Contributor
The purpose was to avoid enqueuing bootstrap writes. In that case it was Not sure if it'll affect performance/memory, just wanted to point it out since it's a significant amount of calls/data being processed. |
Contributor
Author
Good point. I'm working on a refactor of the bootstrapping code, hopefully using coroutines. I'll keep that in mind to profile. |
SergiySW
previously approved these changes
Sep 11, 2020
8a040d0 to
7f7d664
Compare
SergiySW
reviewed
Sep 21, 2020
7f7d664 to
91fd630
Compare
…from nano::socket on to nano::channel_tcp. The intended behavior of nano::socket is to be a thin wrapper around a raw tcp socket with the addition of queueing on a strand plus statistics gathering. The drop policy is part of the channel abstraction which varies depending on the channel used. channel_udp uses the operating system to drop writes it can't service and channel_tcp needs to track the number of outstanding writes to determine what should be dropped. The drop policy was being passed all the way through to nano::socket, punching a hole through the nano::socket abstraction. This commit also removes the concurrency enum as redundant. All socket operations were being serialized through the strand implicitly and the only other function of having a non-concurrent channel was to bypass the socket rate limiter. Non-concurrent users of channel_tcp should manage their own rate limiting if they don't want their buffers dropped. The only users of non-concurrent channels appear to be bootstrapping and this limitation already appears to be respected. This also removes the need for queueing the buffer and callback in a separate container as they're already captured in objects to be executed on the socket's strand. This removes the need to flush the buffers on socket close.
91fd630 to
e0989b8
Compare
guilhermelawless
approved these changes
Sep 21, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit moves the responsibility for limiting writes to a socket from nano::socket on to nano::channel_tcp. The intended behavior of nano::socket is to be a thin wrapper around a raw tcp socket with the addition of queueing on a strand plus statistics gathering.
The drop policy is part of the channel abstraction which varies depending on the channel used. channel_udp uses the operating system to drop writes it can't service and channel_tcp needs to track the number of outstanding writes to determine what should be dropped.
The drop policy was being passed all the way through to nano::socket, punching a hole through the nano::socket abstraction.
This commit also removes the concurrency enum as redundant. All socket operations were being serialized through the strand implicitly and the only other function of having a non-concurrent channel was to bypass the socket rate limiter. Non-concurrent users of channel_tcp should manage their own rate limiting if they don't want their buffers dropped. The only users of non-concurrent channels appear to be bootstrapping and this limitation already appears to be respected.
This also removes the need for queueing the buffer and callback in a separate container as they're already captured in objects to be executed on the socket's strand. This removes the need to flush the buffers on socket close.