Reference changes: Update mio-pollable std-net listener to not require dedicated thread/channels #3819
Conversation
| let mut events = Events::with_capacity(32); | ||
|
|
||
| // spawn a thread to handle events | ||
| std::thread::Builder::new() | ||
| .name("bs_listener".to_string()) | ||
| .spawn(move || { | ||
| // HACK : create variable to move ownership of mio_server to the thread | ||
| // if mio_server is not moved, poll does not receive any event from listener | ||
| let _temp = mio_server; | ||
| let events = Events::with_capacity(32); | ||
| Ok(( | ||
| waker, | ||
| BootstrapTcpListener { | ||
| poll, | ||
| server, | ||
| events, | ||
| _mio_server: mio_server, | ||
| }, | ||
| )) | ||
| } | ||
| } |
There was a problem hiding this comment.
The intent here is to provide the server itself the ability to call a blocking-accept in the main-loop, and be able to determine if it needs to stop
massa-bootstrap/src/server.rs
Outdated
| let (dplx, remote_addr) = match self.listener.accept() { | ||
| Ok(AcceptEvent::NewConnection((dplx, remote_addr))) => (dplx, remote_addr), | ||
| Ok(AcceptEvent::Stop) => break Ok(()), | ||
| Err(e) => { | ||
| error!("bootstrap listener error: {}", e); | ||
| break Err(e); | ||
| } | ||
| }; |
| if let Some(bootstrap_manager) = bootstrap_manager { | ||
| bootstrap_manager.set_listener_stopper(waker); | ||
| } |
There was a problem hiding this comment.
This pattern is problematic....
massa-bootstrap/src/listener.rs
Outdated
| loop { | ||
| poll.poll(&mut events, None).unwrap(); | ||
| impl BSListener for BootstrapTcpListener { | ||
| fn accept(&mut self) -> Result<AcceptEvent, BootstrapError> { |
There was a problem hiding this comment.
i'm not a big fan of no thread the poll event, STOP_LISTENER is called only when we call accept() ?
There was a problem hiding this comment.
the stop-listener and the connection listener both get polled for. You can see the stop-waker being registered here: https://github.com/massalabs/massa/pull/3819/files#diff-75fc005ad20f9d19fed3694ee99b9d4e31d177cc1462e438607552996b8b823bR41-R61
The idea being, is that the main-loop has direct access to the BsListener. in the caso of using a BootstrapTcpListener in this Pr, you have access to:
- a
Poll, that has a waker registered to it - a TcpListener that is registered to the same poll
This means that the TcpListener can be written in such a way that it blocks until either the waker is signaled, or the listener can accept a connection, and is able to differentiate between the two.
There was a problem hiding this comment.
In short: It's as if we could just use a a std::net::TcpListener like so:
let stoppable_listener = std::net::TcpListener::bind(...).with_stopper(&stopper_thing);
let server_thread = std::thread::spawn(|| loop {
let conn = match stopable_listener.stopable_accept() {
Ok(Event::Accept(conn)) => conn,
Ok(Event::stop => {
info!("system-wide stopper invoked. exiting cleanly");
return;
},
Err(e) => panic!("tcp listener errored out"),
};
// do thing with connection
});
// wait untill the stopper thing does its thing so the thread returns out
server_thread.join().unwrap();
info!("server succesfully shutdown gracefully after system-wide stop-signal invoked")There was a problem hiding this comment.
Ok we can go with that;
Maybe stopable_accept can be rename to accept() wich return Err(...) when the stop is called. This allow to remove the Event and has a cleaner result.
There was a problem hiding this comment.
the code i replied with is just an illustrative example. stopable_accept is a hypothetical method that would mirror the behavior as our current accept. No difference, other than the fact that the result can be "stop" or "here's the next connection" instead of just "here's the next connection" (assuming no error)
If something is an Err(...), that implies an incorrect, but handleable, event has occured. Trying to open a file that doesn't exist, for example.
In this case, it's perfectly expected to get a Stop.
Co-authored-by: Modship yeskinokay@gmail.com
|
Sorry about the optional waker... |
the first commit: 2df8da1
split the creation and acceptance into two methods
accepting is a blocking function call analagous to a std-net accept, but with the additon of being waker-interuptable.
the draw-back is that the waker is something that needs to be managed seperately
the second commit is less important, but shows what changes are needed to the listener if we make minimal changes to how the listener plugs into the server.