Skip to content
This repository has been archived by the owner on Aug 29, 2024. It is now read-only.

Add (failing) http.rb wrap test for TCPSocket and SSLSocket #13

Merged
merged 3 commits into from
Oct 9, 2018
Merged

Add (failing) http.rb wrap test for TCPSocket and SSLSocket #13

merged 3 commits into from
Oct 9, 2018

Conversation

ClearlyClaire
Copy link
Contributor

Test to track #12.

One test fails for the wrap/new issue described there, and another fails for a reason I hadn't stumbled upon so far:

Failure/Error: raise RuntimeError, "Trying to close selector with active monitors: #{@monitors.values.inspect}!"

@coveralls
Copy link

coveralls commented Sep 24, 2018

Coverage Status

Coverage increased (+0.5%) to 86.733% when pulling f37e7ca on ThibG:httprb into 52e165e on socketry:master.

@ioquatix
Copy link
Member

The other failure is probably due to http.rb leaking sockets.

@ioquatix
Copy link
Member

That looks good - is that the documented to do it? i.e. response.connection.close

How does http.rb work internally, does it have a cache of connections or something?

@ClearlyClaire
Copy link
Contributor Author

Oops, there are some namespaces issues I didn't notice with the other tests.
The flush things actually fails, it blocks and I'm unsure why… response.connection.close works but it's not something that is explicitly encouraged afaict.

@ioquatix
Copy link
Member

Looks like we need to find out how to support

          NoMethodError:
            undefined method `hostname=' for #<Async::IO::TCPSocket:0x0000000002bcc558>

Not sure if this is part of the stdlib for Ruby or not.

@ClearlyClaire
Copy link
Contributor Author

ClearlyClaire commented Sep 28, 2018

@ioquatix we kind of support this, it's just that HTTP.rb will call ssl_socket_class.new when your wrapping code expects new to be called internally, and wrap to be called instead!

EDIT: To be more precise, Async::IO::SSLSocket isn't a drop-in replacement, as creating it with new as one would usually do will result in it getting a socket instead of the wrapper it expects. Therefore, the subsequent call to hostname= will be on the socket and not on the wrapper/OpenSSL's SSLSocket.

@ioquatix
Copy link
Member

I guess we need to consider how this should work. Whether we should provide some specific Async::IO::Wrap::SSLSocket or implement a constructor for Async::IO::SSLSocket which multiplexes in the same way as TCPSocket.

Because performance of SSLSocket is important, I hesitate to make the constructor complicated for this edge case. But maybe a special wrapper might be appropriate. Both options are kind of annoying.

@ioquatix ioquatix merged commit a59f4e9 into socketry:master Oct 9, 2018
@ioquatix
Copy link
Member

ioquatix commented Oct 9, 2018

I am going to see if I can make this work in a way I'm happy with. Thanks for contributing a failing spec.

@ioquatix
Copy link
Member

ioquatix commented Oct 9, 2018

I've been playing around with this and while it's pretty straight forward with TCPSocket.open, it's less trivial since SSLSocket doesn't support the same method. I feel like it would be nice if they both had a similar open method which would simplify the internal API quite a bit.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants