Remove TCPSocket hard-coding in SSLStream #29
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #29 +/- ##
==========================================
- Coverage 76.80% 76.44% -0.36%
==========================================
Files 2 2
Lines 1095 1104 +9
==========================================
+ Hits 841 844 +3
- Misses 254 260 +6 ☔ View full report in Codecov by Sentry. |
|
I think we should probably release this as a breaking change, or make it backwards compatible (e.g. keep SSLStream as a concrete type that is an alias for NewSSLStream{TCPSocket} (name TBD)) Otherwise dispatch will break, e.g. in cases like |
|
Yeah, I was mulling over the best way to do this too. I hadn't thought of the alias idea, which is interesting. My initial thought was to do a breaking release, then a minor version release of HTTP where we bump up the minimum required to OpenSSL 2.0. Do you think there are other advantages of doing the alias-non-breaking route? |
|
i think a breaking release is probably simplest -- there are only a couple packages depending directly on OpenSSL so a breaking release doesn't seem too disruptive |
| bio_set_read_retry(bio) | ||
| return Cint(0) | ||
| io = bio_get_data(bio) | ||
| if io isa TCPSocket |
There was a problem hiding this comment.
maybe comment about why we have these identical branches (I assume it allows us to avoid a dynamic dispatch in the first branch?)
It makes me wonder how big the perf impact of the dynamic dispatch is if we are switching out TCPSocket (e.g. for an identical implementation that was just of type TCPSocket2)?
| mutable struct SSLStream{T} <: IO | ||
| ssl::SSL | ||
| ssl_context::SSLContext | ||
| rbio::BIO | ||
| wbio::BIO | ||
| io::TCPSocket | ||
| io::T |
There was a problem hiding this comment.
Idea: If we parametrize BIO on the io type it was created with we can have more type stable code downstream and save ourselves some unrolling on the result of bio_get_data. Right now, we'd allocate a bunch for, e.g., a UDPSocket due to dynamic dispatch. The default case when we don't know the IO type for whatever reason can use Any or abstract IO.
I tried to prototype this idea but I hit a problem with how callbacks are used. To get any benefit from this new parametrization, we'd need to call the corresponding specialized method (obviously), but now we use a single method globally via BIOStreamCallbacks, e.g. @cfunction on_bio_stream_read Cint (BIO, Ptr{Cchar}, Cint) but we'd need
@cfunction on_bio_stream_read Cint (BIO{TCPSocket}, Ptr{Cchar}, Cint)
@cfunction on_bio_stream_read Cint (BIO{UDPSocket}, Ptr{Cchar}, Cint)
...
to be used by the BIOMethod object accordingly... which would also mean somehow correlating the type parameter with the BIOMethod and then maintaining keeping the @cfunctions around. I'm not too sure how to best approach that, so I'll just leave this here in case you have some better idea:) I think, instead of the current setup, where there is a single BIOMethod (BIO_STREAM_METHOD) we need an IdDict of them (or a generated function generating and caching them), one for each io type (similar to how we specialize connection pools in HTTP).
There was a problem hiding this comment.
I addressed this issue by having two specialized BIOMethods, one specialize for IO another for TCPStream.
No description provided.