Getting rid of lock().unwrap() #33
Conversation
dr-orlovsky
force-pushed
the
fix/unwrap
branch
3 times, most recently
from
576be6f
to
d2124e6
Compare
src/types.rs
Outdated
|
|
||
| impl<T> From<std::sync::PoisonError<T>> for Error { | ||
| fn from(_: std::sync::PoisonError<T>) -> Self { | ||
| Error::CouldntLockReader |
There was a problem hiding this comment.
I wouldn't use this error here, because CouldntLockReader has a very specific meaning:
Couldn't take a lock on the reader mutex. This means that there's already another reader thread running
If a PoisonError happens there's not much the caller can do to recover, it has to throw away the client and recreate it. I would add an error variant that reflects this better, and write some documentation to let users know what to do in case that error happens.
There was a problem hiding this comment.
I am strongly against panics at runtime which happens not due to app design bugs, especially when you can avoid them in a small cost. What client app can do is to handle the error, terminate the tread gracefully (save the state, notify users, write debug logs, send debug info to developer) and restart the thread.
I am fine introducing a dedicated error variant for these cases.
There was a problem hiding this comment.
Yeah I was suggesting to add a specific variant that reflects the true meaning of the error, and then the user can choose how to handle that variant specifically.
I generally agree with you that it's better to return an error when possible, but on the other end it's very important to ensure that the code can't accidentally end up in an inconsistent/unexpected state: in that regard, the panics on lock() were used to simplify things a little bit.
Basically we chose to panic rather than potentially incur in some kind of logic bug, which I still think it's a better trade-off in many cases.
There was a problem hiding this comment.
I am ok with adding error-specific variant. Or you are against removing unwrap()s on lock()?
There was a problem hiding this comment.
No that's fine, I'm just saying that now that we don't panic we have to be a bit more careful and think about the potential edge cases..
src/stream.rs
Outdated
| self.0.lock().unwrap().read(buf) | ||
| self.0 | ||
| .lock() | ||
| .map_err(|_| io::Error::from(io::ErrorKind::BrokenPipe))? |
There was a problem hiding this comment.
I agree that BrokenPipe is the error variant that more closely maps to a PoisonError, but I would at least log what's happening here first before returning, to avoid confusing the user.
Same for write() and flush() below
There was a problem hiding this comment.
Agree, will update
src/raw_client.rs
Outdated
| sender | ||
| .send(ChannelMessage::WakeUp) | ||
| .expect("Unable to WakeUp a different thread"); | ||
| sender.send(ChannelMessage::WakeUp)?; |
There was a problem hiding this comment.
If we are unable to wake up the thread I guess we should try with all the others before giving up, otherwise all the other threads that are still alive would remain hanging forever waiting for their response.
There was a problem hiding this comment.
Agree, will update
|
If possible will prefer adding error-specific variants after #36 merge, so I can rebase this PR on it and add error displays at the same time |
dr-orlovsky
force-pushed
the
fix/unwrap
branch
from
d2124e6
to
7776ec0
Compare
|
@afilini rebased & fixed according to your comments |
PR removes 16 (!)
unwraps()which are unnecessary and were already covered by an error case.This is quite important for the overall library stability