search

JuliaLang / MbedTLS.jl

Wrapper around mbedtls
Other
41 stars 50 forks source link

MbedException or IOError? #259

Closed gustafsson closed 2 years ago

gustafsson commented 2 years ago

Upon closing an idle connection from the pool in HTTP.jl it checks for any IOErrors and ignores them.

I was going to propose in HTTP.jl that it should also ignore MbedException as I ran into 

Unhandled Task ERROR: MbedTLS error code -80: NET - Connection was reset by peer
Stacktrace:
 [1] closewrite(ctx::MbedTLS.SSLContext)
   @ MbedTLS ~/.julia/packages/MbedTLS/yELM7/src/ssl.jl:205
 [2] close(ctx::MbedTLS.SSLContext)
   @ MbedTLS ~/.julia/packages/MbedTLS/yELM7/src/ssl.jl:175
 [3] monitor_idle_connection(c::HTTP.ConnectionPool.Connection)
   @ HTTP.ConnectionPool ~/.julia/packages/HTTP/fthyG/src/ConnectionPool.jl:266
 [4] (::HTTP.ConnectionPool.var"#2#3"{HTTP.ConnectionPool.Connection})()
   @ HTTP.ConnectionPool ./task.jl:484

But then noticed a move away from MbedException into IOError this commit: https://github.com/JuliaLang/MbedTLS.jl/commit/75f38e91711c749ce18a6977eb67536d04f5b173

Use Base.IOError more consistently for read/write operation failures 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.

Is MbedException about to be removed from MbedTLS?

I'm unfamiliar with this repo so I'm asking here instead of proposing a PR if it would it make sense to make a corresponding change to closewrite as was done to ssl_unsafe_write and ssl_unsafe_read in the commit above?

- throw(MbedException(n))
+ throw(Base.IOError(strerror(n), n))
quinnj commented 2 years ago

Yes, the idea is to move away from MbedException as much as possible. I hesitated to just switch them all over as I wasn't sure it was totally appropriate to always switch to Base.IOError, but let's switch the closewrite case over and we can see if people report other issues. Mind making a PR?