Use Base.IOError more consistently for read/write operation failures #257
Conversation
Previously we might throw ArgumentError, MbedException, or AssertionError. This PR proposes we more closely follow Sockets stdlib behavior by making these errors Base.IOError for consistency. Fixes #188
Codecov Report
@@ Coverage Diff @@
## master #257 +/- ##
==========================================
- Coverage 73.74% 73.73% -0.01%
==========================================
Files 12 12
Lines 716 712 -4
==========================================
- Hits 528 525 -3
+ Misses 188 187 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Instead of MbedException, in line with JuliaLang#257 Fixes JuliaLang#259
| @@ -372,8 +361,7 @@ function ssl_unsafe_read(ctx::SSLContext, buf::Ptr{UInt8}, nbytes::UInt) | |||
| ctx.bytesavailable = 0 ;@😬 "ssl_read ⌛️ $nread" | |||
| return nread | |||
| elseif n < 0 | |||
| ssl_abandon(ctx) | |||
There was a problem hiding this comment.
Was it intentional to drop the call to ssl_abandon here?
There was a problem hiding this comment.
Hmmmm, good question. I think the original intent was just to throw an IOError instead of an MbedException, but we perhaps want the other cleanup that happens in ssl_abandon, so maybe it would be better if we changed ssl_abandon to just throw the IOError instead.
There was a problem hiding this comment.
Cleanup sounds like the right thing to do. I guess the scenario where it would make any difference would be to give a more clear error message were someone to catch the IO-error and then try to read or write again?
I wouldn't expect ssl_abandon to throw though. It would make it harder for me to reason about the control flow in ssl_unsafe_read.
A helper like throw(mbed_ioerr(n)) could come in handy after calling ssl_abandon.
Previously we might throw ArgumentError, MbedException, or AssertionError. This PR proposes we more closely follow Sockets stdlib behavior by making these errors Base.IOError for consistency.
Fixes #188