Conversation
ethanpailes
left a comment
ethanpailes
left a comment
There was a problem hiding this comment.
Thanks! This is a nice quality of life improvement. Just a few comments about style, nothing major.
| // stdin -> sock | ||
| let stdin_to_sock_h = s.spawn(|| -> anyhow::Result<()> { | ||
| let stdin_to_sock_h = thread::Builder::new() | ||
| .name("stdin_to_sock".to_string()) |
There was a problem hiding this comment.
Let's match the style for thread names from shell.rs and use -> rather than to, so for this one it would be stdin->sock
libshpool/src/protocol.rs
Outdated
| write_client_stream.flush().context("flushing client")?; | ||
| } | ||
| }); | ||
| }).unwrap(); |
There was a problem hiding this comment.
Let's do .map_err(|e| anyhow!("spawning stdin->sock: {:?}", e)?; instead of unwrapping here.
There was a problem hiding this comment.
Should I use expect() on that to get the proper value? That will still panic, just hopefully with a better message?
There was a problem hiding this comment.
I would like to avoid panics whenever possible, and where we have them they should be justified by a comment. Looking at it some more, I think we could wind up blocking forever if the first thread succeeds in launching and the second thread fails, which results in returning an error. Also the thread::scope call currently does not bubble errors up so it would just be dropped if we bubble the error up (though that should be easy to fix).
I'm happy to keep the panics if they use expect() to label themselves and have comments explaining why it is justified to panic in order to avoid blocking / because not being able to spawn a thread is catastrophic enough that we shoudl just give up.
| // sock -> stdout | ||
| let sock_to_stdout_h = s.spawn(|| -> anyhow::Result<()> { | ||
| let sock_to_stdout_h = thread::Builder::new() | ||
| .name("sock_to_stdout".to_string()) |
libshpool/src/protocol.rs
Outdated
| } | ||
| } | ||
| }); | ||
| }).unwrap(); |
This would have sped up debugging shell-pool#243 had it already been implemented, and other threads are named, so I'm guessing the lack of a name is an oversight.
jasom
force-pushed
the
feat/name-io-threads
branch
from
433d96f to
c49b72f
Compare
2 participants
This would have sped up debugging #243 had it already been implemented, and other threads are named, so I'm guessing the lack of a name is an oversight.