Binder - add/fix ssl 0.0.0.0/localhost logging, use ternary operator #2533
Conversation
MSP-Greg
force-pushed
the
binder-ssl-localhost
branch
from
0d6e19b
to
4618e54
Compare
|
I believe that current code may show binds multiple times if there are multiple binds. Just pushed an update that should fix that... |
| io = add_ssl_listener uri.host, uri.port, ctx | ||
| logger.log "* Listening on #{str}" | ||
|
|
||
| @ios[ios_len..-1].each do |i| |
There was a problem hiding this comment.
Is the index necessary? are you just trying to reverse it? just use reverse_each
There was a problem hiding this comment.
No, not reverse. If there are multiple binds, I believe the current loop will print multiple entries?
There was a problem hiding this comment.
I think I'm missing some context. What problem does this PR solve?
There was a problem hiding this comment.
See two tests that had skips removed. Only pertains to '0.0.0.0' or 'localhost' binds
There was a problem hiding this comment.
I think I'm following this now. In this specific case, we want to print that we're listening on all newly-added entries to @ios.
In that case, I think it might be more straightforward for add_ssl_listener to return all instances of Puma::MiniSSL::Server it created.
ssl_servers = add_ssl_listener uri.host, uri.port, ctx
ssl_servers.each do |server|
addr = loc_addr_str server
ssl_qry = str[/\?.+\z/]
logger.log "* #{log_msg} on ssl://#{addr}#{ssl_qry}"
endThere was a problem hiding this comment.
IDK.
I did just notice that ssl_qry = str[/\?.+\z/] can be moved out of the loop.
Note that add_ssl_listener currently returns the TCP server 'behind' the SSL server. The SSL server is added to @ios. That may not be needed, as I think it can also change if MiniSSL::Server does a better job of barking like a TCP server. I think I added MiniSSL::Server#closed? for that purpose in another PR.
A while ago, someone mentioned refactoring/rewriting BInder, so I haven't really looked at it. I've also got code for abstract unix sockets, and it runs with restart ok...
So, maybe wait on more serious Binder changes with Puma 6? Just a thought...
There was a problem hiding this comment.
Yeah, happy to defer a refactor to a future PR 👍
There was a problem hiding this comment.
I moved the ssl_qry statement and also removed the next unless *Server === i statements, as with the @iOS length handling, they're no longer needed...
There was a problem hiding this comment.
Both changes look good to me 👍
There was a problem hiding this comment.
@cjlarose Thanks for looking at it, @nateberkopec also...
|
That iterator looks weird otherwise seems fine |
|
Insert the following line at Run with the current PR, output is below, showing three IPv4 and 3 IPv6 addresses: Now, remove the 'array' part from |
MSP-Greg
force-pushed
the
binder-ssl-localhost
branch
from
4618e54
to
c9b78d0
Compare
Heh, I didn't get it before I looked at the diff, I'm more used to the term "ternary operator" |
test_binder.rb - remove skips for: test_correct_zero_port_ssl test_logs_all_localhost_bindings_ssl
MSP-Greg
force-pushed
the
binder-ssl-localhost
branch
from
c9b78d0
to
eb469af
Compare
MSP-Greg
changed the title
test_binder.rb - remove skips for: test_correct_zero_port_ssl test_logs_all_localhost_bindings_ssl
Description
Binder - add/fix ssl 0.0.0.0/localhost logging, use tertiary
test_binder.rb - remove skips for:
test_correct_zero_port_ssl
test_logs_all_localhost_bindings_ssl
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.