fix some subtle bugs with error handling and cleanup#243
fix some subtle bugs with error handling and cleanup#243quinnj merged 1 commit intoJuliaLang:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #243 +/- ##
==========================================
+ Coverage 71.81% 72.00% +0.18%
==========================================
Files 12 12
Lines 699 700 +1
==========================================
+ Hits 502 504 +2
+ Misses 197 196 -1
Continue to review full report at Codecov.
|
|
Looks like |
TLS streams can be a bit odd, since they mostly ignore errors on the underlying (unencrypted) stream. Clean up some of that error handling (dealt with in wait_for_encrypted_data, so this actually ends up not passing through mbedtls even though we still could feed it through there), improve flow control reliability (avoids damaging the underlying stream), implement new `Base.closewrite` API
|
yes, but I don't bother calling |
|
I bisected the HTTP.jl error from JuliaWeb/HTTP.jl#896 to this commit. |
|
Yeah, I think we better revert this change unless @vtjnash would have time to look into it sometime soon. |
|
You will run into other more frequent deadlocks now if you attempt to revert this, since it does fix some problematic assumptions about stream behaviors. The HTTP says |
| Cint(MBEDTLS_ERR_NET_CONN_RESET) | ||
| if n == 0 ;@🤖 "f_recv $(isopen(bio) ? "WANT_READ" : "RECV_FAILED")" | ||
| return isreadable(bio) ? Cint(MBEDTLS_ERR_SSL_WANT_READ) : | ||
| Cint(MBEDTLS_ERR_NET_RECV_FAILED) |
There was a problem hiding this comment.
you might need to feed through the actual error here, or indicate to mbedtls somehow if it reached eof on the underlying bio
There was a problem hiding this comment.
Does it support MBEDTLS_ERR_SSL_CONN_EOF? When I looked at the source, it wasn't clear to me how you indicated TCP FIN to mbedtls (before v1.3 of the TLS standard, this would have been an invalid termination, but was generally permitted in many clients and expected by some popular servers, so I assume mbedtls would support this)
|
The normal |
|
Could you write a test for what you think is wrong? This PR passes all tests, and additionally passes the stricter verification for stream-implementation correctness that is now on Julia master |
|
Found it and added the missing handling + tested now #248 |
3 participants
TLS streams can be a bit odd, since they mostly ignore errors on the
underlying (unencrypted) stream. Clean up some of that error handling
(dealt with in wait_for_encrypted_data, so this actually ends up not
passing through mbedtls even though we still could feed it through
there), improve flow control reliability (avoids damaging the underlying
stream), implement new
Base.closewriteAPI