binder.rb - fix using both UNIX and TCP listeners

[MSP-Greg]

See issue #1986

1st Commit - 'binder.rb - fix double binds and misc'

1. Fix mult binds when unix is first, followed by ip bind. See issue Error after binding to socket and port in one config #1986.

2. When localhost is used for ssl or tcp, output actual ip's bound to.

3. Fix issues with IPv6 bind logging (brackets).

4. Move ssl context initialization code from Binder#parse to new method, #ssl_ctx_init.

2nd Commit - 'test_binder.rb - add double bind tests, misc'

1. Add test combinations of multiple binds, localhost, port 0, etc.

2. Allow most tests using ssl binders to run on JRuby, including MiniSSL::Context#no_tlsv* tests, which all were previously only run on MRI Ruby.

3. Close sockets created by Binder#parse.

4. Refactor code.

5. Some redundant asserts. Could use cleanup to make more orthogonal.

[MSP-Greg]

re forced push, I think skipped? may only work in teardown?

[nateberkopec]

Closing in favor of #1989

[nateberkopec]

that one got too complicated, coming back to this

[MSP-Greg]

@nateberkopec

I spent some more time looking at this, and decided to add tests for activated & inherited sockets.
Turned into a rabbit hole kind of thing, but I think I've got it working. Also, added needed cleanup for sockets (Binder @iOS, etc).

See the files changed in https://github.com/MSP-Greg/puma/tree/unix-and-tcp-listeners-t. Last four commits, except for 'local test rakefile, Action' (Rakefile_test lets me run locally tests on a branch other than 'master' in my fork, and the changes to ruby.yml allow me to run Actions given the same).

I could separate it into another PR, but that may have merge issues if anything is changed in the first. Thoughts?

Question - Binder#import_from_env loads @inherited_fds and @activated_sockets.

Inherited sockets (PUMA_INHERIT_\d) have an exact fd passed via individual ENV settings, and the ENV settings are created by Puma.

Activated sockets ( systemd LISTEN_FDS) have a single number passed in, and fd's are looped looking for the matching uri. But, binder fd(s) may not be sequential above STDERR (3, 4, etc).

The code (in Binder#parse) below closes/deletes any unused fd's in @activated_sockets:

puma/lib/puma/binder.rb

Lines 250 to 260 in 079b284

	# Also close any unused activated sockets
	@activated_sockets.each do |key, sock|
	logger.log "* Closing unused activated socket: #{key.join ':'}"
	begin
	sock.close
	rescue SystemCallError
	end
	# We have to unlink a unix socket path that's not being used
	File.unlink key[1] if key[0] == :unix
	end
	end

Aren't we removing fd's in @activated_sockets that we possibly shouldn't?

[nateberkopec]

@MSP-Greg re: those questions, please contact me via the email address on my profile

[dentarg]

Why discuss offline instead of GitHub?

[MSP-Greg]

Patrik,

Why discuss offline instead of GitHub?

OFF TOPIC

I think I can state that recently both Nate & I said something similar to "let's focus on a few existing, fixable issues and get a point release out", referring to this and the systemd issues. I think we both also started on things that morphed into larger, more involved changes, which went beyond what is needed for the point release.

My post above was partially in frustration, as some of the work I did in my fork stabilized test_binder.rb, but I'd need to cherry pick those changes back into this PR, and take the additional test work (and its changes to the lib files) for another PR.

Said another way, sometimes it's very frustrating to work on A (this PR), then work on B (my branch), and realize that B needs to be divided, one part moving back into A, and the balance saved for later. Been, there, done that, and it requires a lot of concentration, as A and B are stable, but dividing them may make A unstable until the 'division' is 'perfect'.

Summing up, we're doing a 'bug fix' push. Next push is for refactoring, better test coverage, improvements, etc.

So, I'm going to start working on that division. Nate & I have not communicated, so I'm not speaking for him in any way.

Sorry for being off-topic, Greg

[MSP-Greg]

Updated.

[MSP-Greg]

With the last commit, most tests using ssl binders run with JRuby, which is a change from the previous version, where very few ran.

See:

https://travis-ci.org/puma/puma/jobs/590983851#L411
and
https://travis-ci.org/puma/puma/jobs/590983851#L538

[MSP-Greg]

Luckily, last Travis CI build allowed both JRuby jobs to run TestBinder before they locked up.

All passed, including the MiniSSL::Context#no_tlsv* tests, which were previously only run on MRI Ruby.

9.2.8.0 - https://travis-ci.org/puma/puma/jobs/591284029#L352-L365
head - https://travis-ci.org/puma/puma/jobs/591284032#L414-L427

[nateberkopec]

Ok, now THIS one's too complicated. We need just tests + a simple hack fix for 4.2.1, not a full refactor (which is also what I attempted too). I know it's tempting because it's obviously going to help a lot of the bugs we're seeing around activation. The problem with a full refactor here is that the test coverage on binders is only ok and it's certainly nonexistent for inheriting/activating.

[MSP-Greg]

The 'double path' bug has been fixed

My current version of test/test_binder.rb is compatible with master, but I'd like to add more test coverage. So, I can either update this, or close this and create another PR.

Thoughts?

@nateberkopec - your call

[nateberkopec]

Closing in favor of #1989