-
Notifications
You must be signed in to change notification settings - Fork 50
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
Use Base.IOError in closewrite #260
Conversation
Instead of MbedException, in line with JuliaLang#257 Fixes JuliaLang#259
Codecov Report
@@ Coverage Diff @@
## master #260 +/- ##
==========================================
- Coverage 73.77% 73.56% -0.21%
==========================================
Files 12 12
Lines 713 715 +2
==========================================
Hits 526 526
- Misses 187 189 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
I stumbled upon a related question when making this change: |
@@ -202,7 +202,7 @@ function closewrite(ctx::SSLContext) | |||
"never returns ...WANT_READ/WRITE." | |||
elseif n != 0 | |||
ssl_abandon(ctx) |
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.
Yeah, as I commented on in the related PR, the problem here is ssl_abandon
is going to throw before we're able to throw the IOError
on the next line.
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.
Thanks that make sense. However ssl_abandon
doesn't throw in my scenario (#259) where I reliably end up here with n = -80, strerror(n) = NET - Connection was reset by peer
.
So the issue is resolved for me.
Would you like me to replace MbedException(n)
with Base.IOError(strerror(n), n)
in ssl_abandon
anyways?
Should we also make the swap in handshake
while at it? That's the last remaining reference to MbedException
in this file.
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.
Yeah, let's switch them all to IOError; thanks.
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.
Thanks!
Instead of MbedException, in line with #257
Fixes #259