core/src/connection: Bound task event buffer during conn establishment #1501
Conversation
Introduce unit test ensuring that the event buffer of a `Task` has a limit and once exceeded that the `Task` sends an error back to the manager and eventually fails.
This commit adds a size limit to the event buffer that is filled during connection establishment and forwarded in bulk to the handler once the connection is established. When the size limit is reached an `EventBufferLimitReached` error is triggered and send to the manager which will in turn drop its connection to the task signaling it to shutdown. The above buffer is already bound by the connection establishment timeout. This commit adds an additional size bound to the buffer.
| @@ -35,6 +35,8 @@ use futures::{prelude::*, channel::mpsc, stream}; | |||
| use std::{pin::Pin, task::Context, task::Poll}; | |||
| use super::ConnectResult; | |||
|
|
|||
| const MAX_EVENT_BUFFERING: usize = 1000; | |||
There was a problem hiding this comment.
What would be a good limit? 1_000 might be a bit conservative.
|
I am wondering if it is actually still necessary to buffer these events in the background task at all, since the "take over" commands sent to pending connections in the context of the single-connection-per-peer constraint have been removed. The API |
|
I agree with @romanb. Being able to send events to connections that haven't been established yet was probably a mistake. |
|
Thanks for the feedback @romanb @tomaka. To summarize we have two alternative approaches both relieving us off the need to buffer events in the first place.
In both cases one would still signal a Do you have preferences on (1) or (2)? |
Yes.
I'd just go with 2. |
|
If
I'm not sure what you mean by that w.r.t. this PR? Do you mean what a |
|
Replaced by #1527. Thanks for the input! |
This commit adds a size limit to the event buffer that is filled during
connection establishment and forwarded in bulk to the handler once the
connection is established.
When the size limit is reached an
EventBufferLimitReachederror istriggered and send to the manager which will in turn drop its connection
to the task signaling it to shutdown.
The above buffer is already bound by the connection establishment
timeout. This commit adds an additional size bound to the buffer.
Fixes #1465.