Skip to content
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.

Sign up for GitHub

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

Remove TCPSocket hard-coding in SSLStream #29

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Remove TCPSocket hard-coding in SSLStream #29

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b8b0369
Remove TCPSocket hard-coding in SSLStream
quinnj Jul 13, 2023
6aa00bd
Fix merge
quinnj Jul 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 27 additions & 13 deletions src/ssl.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,22 @@ bio_clear_flags(bio::BIO) = bio_set_flags(bio, 0x00)
function on_bio_stream_read(bio::BIO, out::Ptr{Cchar}, outlen::Cint)
try
bio_clear_flags(bio)
io = bio_get_data(bio)::TCPSocket
n = bytesavailable(io)
if n == 0
bio_set_read_retry(bio)
return Cint(0)
io = bio_get_data(bio)
if io isa TCPSocket
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

n = bytesavailable(io)
if n == 0
bio_set_read_retry(bio)
return Cint(0)
end
unsafe_read(io, out, min(UInt(n), outlen))
else
n = bytesavailable(io)
if n == 0
bio_set_read_retry(bio)
return Cint(0)
end
unsafe_read(io, out, min(UInt(n), outlen))
end
unsafe_read(io, out, min(UInt(n), outlen))
return Cint(min(n, outlen))
catch e
# we don't want to throw a Julia exception from a C callback
Expand All @@ -51,8 +60,13 @@ end

function on_bio_stream_write(bio::BIO, in::Ptr{Cchar}, inlen::Cint)::Cint
try
io = bio_get_data(bio)::TCPSocket
written = unsafe_write(io, in, inlen)
bio_clear_flags(bio)
io = bio_get_data(bio)
if io isa TCPSocket
written = unsafe_write(io, in, inlen)
else
written = unsafe_write(io, in, inlen)
end
return Cint(written)
catch e
# we don't want to throw a Julia exception from a C callback
Expand Down Expand Up @@ -373,12 +387,12 @@ end
"""
SSLStream.
"""
mutable struct SSLStream <: IO
mutable struct SSLStream{T} <: IO
ssl::SSL
ssl_context::SSLContext
rbio::BIO
wbio::BIO
io::TCPSocket
io::T
Comment on lines +390 to +395
Copy link
Contributor

@Drvi Drvi Jul 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

@grlap grlap Aug 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I addressed this issue by having two specialized BIOMethods, one specialize for IO another for TCPStream.

#27

# used in `eof` where we want the call to `eof` on the underlying
# socket and the SSL_peek call that processes bytes to be seen
# as one "operation"
Expand All @@ -395,16 +409,16 @@ mutable struct SSLStream <: IO
peekbytes::Base.RefValue{Csize_t}
closed::Bool

function SSLStream(ssl_context::SSLContext, io::TCPSocket)
function SSLStream(ssl_context::SSLContext, io::T) where {T <: IO}
# Create a read and write BIOs.
bio_read::BIO = BIO(io; finalize=false)
bio_write::BIO = BIO(io; finalize=false)
ssl = SSL(ssl_context, bio_read, bio_write)
return new(ssl, ssl_context, bio_read, bio_write, io, ReentrantLock(), ReentrantLock(), Ref{Csize_t}(0), Ref{Csize_t}(0), Ref{UInt8}(0x00), Ref{Csize_t}(0), false)
return new{T}(ssl, ssl_context, bio_read, bio_write, io, ReentrantLock(), ReentrantLock(), Ref{Csize_t}(0), Ref{Csize_t}(0), Ref{UInt8}(0x00), Ref{Csize_t}(0), false)
end
end

SSLStream(tcp::TCPSocket) = SSLStream(SSLContext(OpenSSL.TLSClientMethod()), tcp)
SSLStream(tcp::IO) = SSLStream(SSLContext(OpenSSL.TLSClientMethod()), tcp)

# backwards compat
Base.getproperty(ssl::SSLStream, nm::Symbol) = nm === :bio_read_stream ? ssl : getfield(ssl, nm)
Expand Down