server.rb - properly cork & uncork ssl clients #2550
Conversation
lib/puma/server.rb
Outdated
| Socket.const_defined?(:IPPROTO_TCP) && | ||
| Socket.const_defined?(:TCP_CORK) | ||
| end | ||
|
|
||
| # :nodoc: | ||
| # @version 5.0.0 | ||
| def closed_socket_supported? | ||
| RbConfig::CONFIG['host_os'] =~ /linux/ && | ||
| RbConfig::CONFIG['host_os'].include?('linux') && |
There was a problem hiding this comment.
What's the motivation for the change here?
There was a problem hiding this comment.
General code improvement.
- If you need a boolean, choose a method that returns one. I think that was the reason
String#match?was added in Ruby 2.4. - Use string methods if a RegExp test isn't needed. If and when macOS supports 'cork' (and hence
/linux|darwin/), it can be a string method.
Off topic: A long time ago, my first 'significant' contribution to 'OS Ruby' was working on building & testing Windows/MinGW Ruby. Since then, I've always kept an eye on the commits to ruby/ruby. There are a lot of commits there that include small code changes like the change above.
I can revert and/or add a 'History.md' line
There was a problem hiding this comment.
I looked at this a bit more, and SOCKET::TCP_CORK is not defined in Windows and macOS. So, a better solution might be to add HAS_TCP_CORK to detect.rb and add a conditional to the method calls? So:
cork_socket io
uncork_socket iowould change to:
cork_socket(io) if HAS_TCP_CORK
uncork_socket(io) if HAS_TCP_CORKNot sure, ::Puma::HAS_TCP_CORK might be a bit faster, but simple conditionals run much quick that method calls, etc...
There was a problem hiding this comment.
Ran ruby -rsocket -e 'puts Socket.constants.sort' in CI, and updated the code based on that. The needed constants are not defined in Windows or macOS, so removed the linux detection...
MSP-Greg
force-pushed
the
request-ssl-cork
branch
from
894aadb
to
3a87cd7
Compare
MSP-Greg
changed the title
| @@ -19,7 +21,6 @@ | |||
| * Fix phased restart errors related to nio4r gem when using the Puma control server ([#2516]) | |||
| * Add `#string` method to `Puma::NullIO` ([#2520]) | |||
| * Fix binding via Rack handler to IPv6 addresses ([#2521]) | |||
| * Require rack/common_logger explicitly if :verbose is true ([#2547]) | |||
Description
Server##cork_socket&Server##uncork_socketwork on TCPSockets, but when using an ssl client, a MiniSSL::Socket is passed to them.Use 'to_io', which is available with all 'socket objects' used in Puma.
I haven't worked with wireshark for a while, and that's probably the only way to see a difference?
Your checklist for this pull request
[changelog skip]or[ci skip]to the pull request title.[ci skip]to the title of the PR.#issue" to the PR description or my commit messages.