-
Notifications
You must be signed in to change notification settings - Fork 980
Refactor stratum server leak handling #3876
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
base: staging
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,5 +13,6 @@ wallet_data | |
| wallet/db | ||
| .idea/ | ||
| .vscode/ | ||
| .venv/ | ||
| /node* | ||
| result | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,9 +15,8 @@ | |
| //! Mining Stratum Server | ||
|
|
||
| use futures::channel::mpsc; | ||
| use futures::pin_mut; | ||
| use futures::{SinkExt, StreamExt, TryStreamExt}; | ||
| use tokio::net::TcpListener; | ||
| use futures::{SinkExt, StreamExt}; | ||
| use tokio::net::{TcpListener, TcpStream}; | ||
| use tokio::runtime::Runtime; | ||
| use tokio_util::codec::{Framed, LinesCodec}; | ||
|
|
||
|
|
@@ -599,72 +598,92 @@ impl Handler { | |
|
|
||
| // ---------------------------------------- | ||
| // Worker Factory Thread Function | ||
|
|
||
| struct WorkerCleanup { | ||
| worker_id: usize, | ||
| workers: Arc<WorkersList>, | ||
| } | ||
|
|
||
| impl Drop for WorkerCleanup { | ||
| fn drop(&mut self) { | ||
| self.workers.remove_worker(self.worker_id); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The worker is removed from the map, but its worker_stats entry remains forever. Could we remove or reuse these entries during cleanup? |
||
| info!("Worker {} disconnected", self.worker_id); | ||
| } | ||
| } | ||
|
|
||
| async fn handle_connection(socket: TcpStream, handler: Arc<Handler>) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we add lifecycle tests for this? The existing Stratum tests only cover JSON serialization and never reach this code. |
||
| let (tx, mut rx) = mpsc::unbounded(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since #3889 is closed, I think the limits should be included here. Idle connections otherwise stay open indefinitely and the unbounded queue can keep growing. Could we add an idle timeout, a connection limit, and a bounded queue? |
||
| let worker_id = handler.workers.add_worker(tx); | ||
| let _cleanup = WorkerCleanup { | ||
| worker_id, | ||
| workers: handler.workers.clone(), | ||
| }; | ||
|
|
||
| if let Ok(addr) = socket.peer_addr() { | ||
| info!("Worker {} connected from {}", worker_id, addr); | ||
| } else { | ||
| info!("Worker {} connected", worker_id); | ||
| } | ||
|
|
||
| let mut framed = Framed::new(socket, LinesCodec::new()); | ||
|
|
||
| loop { | ||
| tokio::select! { | ||
| incoming = framed.next() => { | ||
| match incoming { | ||
| Some(Ok(line)) => { | ||
| let request: RpcRequest = match serde_json::from_str(&line) { | ||
| Ok(req) => req, | ||
| Err(e) => { | ||
| error!("error serializing line: {}", e); | ||
| break; | ||
| } | ||
| }; | ||
| let resp = handler.handle_rpc_requests(request, worker_id); | ||
| handler.workers.send_to(worker_id, resp); | ||
| } | ||
| Some(Err(e)) => { | ||
| error!("error reading line: {}", e); | ||
| break; | ||
| } | ||
| None => break, | ||
| } | ||
| } | ||
| outgoing = rx.next() => { | ||
| match outgoing { | ||
| Some(line) => { | ||
| if let Err(e) = framed.send(line).await { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While framed.send() is blocked, the socket is no longer read. In my test, this kept a half closed socket alive, while the previous split reader and writer closed it immediately. Could we keep reads and writes independent? |
||
| error!("error writing line: {}", e); | ||
| break; | ||
| } | ||
| } | ||
| None => break, | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn accept_connections(listen_addr: SocketAddr, handler: Arc<Handler>) { | ||
| info!("Start tokio stratum server"); | ||
| let task = async move { | ||
| let listener = TcpListener::bind(&listen_addr).await.unwrap_or_else(|_| { | ||
| panic!("Stratum: Failed to bind to listen address {}", listen_addr) | ||
| }); | ||
| let server = async_stream::stream! { | ||
| loop { | ||
| match listener.accept().await { | ||
| Ok((socket, _)) => yield socket, | ||
| Err(e) => { | ||
| error!("accept error = {:?}", e); | ||
| continue; | ||
| } | ||
| loop { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was the last use of async stream. Could we also remove the dependency from servers/Cargo.toml? |
||
| match listener.accept().await { | ||
| Ok((socket, _)) => { | ||
| let handler = handler.clone(); | ||
| tokio::spawn(async move { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we make the listener and connection tasks cancellable and keep their handles? That would also make clean start and stop support through the Owner API possible later. |
||
| handle_connection(socket, handler).await; | ||
| }); | ||
| } | ||
| Err(e) => { | ||
| error!("accept error = {:?}", e); | ||
| continue; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we add a short backoff for accept errors? With EMFILE, this loop would otherwise spin and flood the log. |
||
| } | ||
| } | ||
| } | ||
| .for_each(move |socket| { | ||
| let handler = handler.clone(); | ||
| async move { | ||
| // Spawn a task to process the connection | ||
| let (tx, mut rx) = mpsc::unbounded(); | ||
|
|
||
| let worker_id = handler.workers.add_worker(tx); | ||
| info!("Worker {} connected", worker_id); | ||
|
|
||
| let framed = Framed::new(socket, LinesCodec::new()); | ||
| let (mut writer, mut reader) = framed.split(); | ||
|
|
||
| let h = handler.clone(); | ||
| let read = async move { | ||
| while let Some(line) = reader | ||
| .try_next() | ||
| .await | ||
| .map_err(|e| error!("error reading line: {}", e))? | ||
| { | ||
| let request = serde_json::from_str(&line) | ||
| .map_err(|e| error!("error serializing line: {}", e))?; | ||
| let resp = h.handle_rpc_requests(request, worker_id); | ||
| h.workers.send_to(worker_id, resp); | ||
| } | ||
|
|
||
| Result::<_, ()>::Ok(()) | ||
| }; | ||
|
|
||
| let write = async move { | ||
| while let Some(line) = rx.next().await { | ||
| writer | ||
| .send(line) | ||
| .await | ||
| .map_err(|e| error!("error writing line: {}", e))?; | ||
| } | ||
|
|
||
| Result::<_, ()>::Ok(()) | ||
| }; | ||
|
|
||
| let task = async move { | ||
| pin_mut!(read, write); | ||
| futures::future::select(read, write).await; | ||
| handler.workers.remove_worker(worker_id); | ||
| info!("Worker {} disconnected", worker_id); | ||
| }; | ||
| tokio::spawn(task); | ||
| } | ||
| }); | ||
| server.await | ||
| }; | ||
|
|
||
| let rt = Runtime::new().unwrap(); | ||
|
|
||
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 looks unrelated to the Stratum change. Could we leave it out of this PR?