Skip to content

Move channel drop policy off of nano::socket and remove channel concurrency value as redundant#2918

Merged
clemahieu merged 1 commit intodevelopfrom
tcp_write_cleanup
Sep 22, 2020
Merged

Move channel drop policy off of nano::socket and remove channel concurrency value as redundant#2918
clemahieu merged 1 commit intodevelopfrom
tcp_write_cleanup

Conversation

@clemahieu
Copy link
Copy Markdown
Contributor

@clemahieu clemahieu commented Sep 8, 2020

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.

@guilhermelawless guilhermelawless added this to the V22.0 milestone Sep 8, 2020
@guilhermelawless guilhermelawless added the quality improvements This item indicates the need for or supplies changes that improve maintainability label Sep 8, 2020
SergiySW
SergiySW previously approved these changes Sep 8, 2020
@cryptocode
Copy link
Copy Markdown
Contributor

cryptocode commented Sep 8, 2020

This commit also removes the concurrency enum as redundant.

The purpose was to avoid enqueuing bootstrap writes. In that case it was async_write->handler->async_write->... (such as bulk_pull_server::send_next), while now it'll be post (strand queuing)->async_write->handler->post->async_write->...

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.

@clemahieu
Copy link
Copy Markdown
Contributor Author

es. In that case it was async_write->handler->async_write->... (such as bulk_pull_server::send_next), while now it'll be post (stran

Good point. I'm working on a refactor of the bootstrapping code, hopefully using coroutines. I'll keep that in mind to profile.

SergiySW
SergiySW previously approved these changes Sep 11, 2020
@clemahieu clemahieu force-pushed the tcp_write_cleanup branch 4 times, most recently from 8a040d0 to 7f7d664 Compare September 21, 2020 09:54
…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.
Copy link
Copy Markdown
Contributor

@guilhermelawless guilhermelawless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TSAN and ASAN passing, tested a short bootstrap on Debug also. Great change!

@clemahieu clemahieu merged commit a11c539 into develop Sep 22, 2020
@zhyatt zhyatt deleted the tcp_write_cleanup branch September 21, 2021 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

quality improvements This item indicates the need for or supplies changes that improve maintainability

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants