Allow setting TCP low_latency with SSL listener, minor fixes

[MSP-Greg]

Description

Started with the following warnings in Ruby 2.4:

lib/puma/binder.rb:254: warning: assigned but unused variable - optimize_for_latency
lib/puma/request.rb:334: warning: shadowing outer local variable - part
lib/puma/request.rb:343: warning: shadowing outer local variable - part
lib/puma/request.rb:357: warning: shadowing outer local variable - part
lib/puma/request.rb:365: warning: shadowing outer local variable - part

Fixed the above, rather trivial.

Then, while looking at the 'optimize_for_latency' warning, discovered that the TCPServer started for an SSL connection was not passed the 'low_latency' argument.

Fixed, and added tests to test_binder.rb & test_config.rb, which is the last commit. Cherry-pick the test commits to master, it shows the following three failures:

  1) Failure:
TestBinderParallel#test_binder_ssl_parses_false_low_latency [/mnt/c/Greg/GitHub/puma/test/test_binder.rb:248]:
Expected true to not be truthy.

48 runs, 96 assertions, 1 failures, 0 errors, 0 skips


  1) Failure:
TestConfigFile#test_ssl_bind_with_low_latency_true [/mnt/c/Greg/GitHub/puma/test/test_config.rb:172]:
Expected false to be truthy.

  2) Failure:
TestConfigFile#test_ssl_bind_with_low_latency_false [/mnt/c/Greg/GitHub/puma/test/test_config.rb:188]:
Expected false to be truthy.

53 runs, 81 assertions, 2 failures, 0 errors, 3 skips

Your checklist for this pull request

• I have reviewed the guidelines for contributing to this repository.
• I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
• My pull request is 100 lines added/removed or less so that it can be easily reviewed.
• If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
• If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
• I have updated the documentation accordingly.
• All new and existing tests passed, including Rubocop.

[nateberkopec]

I think I would prefer:

Suggested change

	opt = params.key?('low_latency') && params['low_latency'] != 'false'
	low_latency = params['low_latency'] != 'false'
	io = add_ssl_listener uri.host, uri.port, ctx, low_latency, backlog

[MSP-Greg]

I can change the variable name from opt to low_latency, but it is false by default, so the conditional should stay the same. Also, the code added is the same as line 161, which is used with tcp connections:

puma/lib/puma/binder.rb

Line 161 in 3f4e3c8

opt = params.key?('low_latency') && params['low_latency'] != 'false'

Thoughts?

[MSP-Greg]

Added a commit renaming the variable from opt to low_latency

[nateberkopec]

Style comment but otherwise 👍

[MSP-Greg]

@nateberkopec

Been doing a deep dive into specs, etc. The main issue here is the line below, in particular optimize_for_latency = true, which is okay in a method definition, but not in a method call:

puma/lib/puma/binder.rb

Line 254 in 6da32ca

io = add_ssl_listener uri.host, uri.port, ctx, optimize_for_latency = true, backlog

So, that should be fixed, but then the question is how. Currently, that uses a value of true (which is the latency setting for the SSL socket's 'parent' TCP connection), but when using a TCP listener, the default value is false. So, if we change it to false, it could be considered a breaking change.

Also, the DSL helper functions for SSL (DLS.ssl_bind_str & DSL#bind_str) do not pass this setting on, so it would be best to fix that (which this PR currently does not).

I did some quick benchmarks, and saw very little change with various values of low_latency with either SSL or TCP connections. I'll probably do some more testing, but not a high priority.

---

Finally, given the odd current behavior, we should leave the SSL default as true, which would use the change you suggested, and add it to the DSL options for SSL?

JFYI, I was the one who merged this...

[nateberkopec]

Finally, given the odd current behavior, we should leave the SSL default as true, which would use the change you suggested, and add it to the DSL options for SSL?

Yup, sounds good.

[MSP-Greg]

Updated as per above. Or, the low_latency setting for the TCPServer 'behind' the SSLServer can now be set either via DSL#ssl_bind or via a bind string.