-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Binder refactor #1989
Binder refactor #1989
Conversation
lib/puma/binder.rb
Outdated
when UNIXServer | ||
["unix", i.local_address] | ||
when Puma::MiniSSL::Server | ||
["ssl", i.to_io.local_address] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has the advantage of logging port 0 binds with SSL correctly too.
@MSP-Greg thoughts? |
Oh, on windows, UNIXServer isn't even defined? |
I need to think about how this affects inherited listeners, and whether or not I should be iterating through |
eabef54
to
f892e80
Compare
I don't think it matters whether or not I'm iterating through listeners or ios. Unfortunately, those are supposed to be the same data AFAICT. I think we should probably just have one collection in binders (merging ios and listeners), a collection of a new value object (call it Binding?). That would make a lot of stuff a lot nicer. I don't really want to do that refactoring right now but I think I have to in order to log inherited sockets once rather than twice (which this will now do). |
58e4c69
to
4da434a
Compare
This is ending up a lot more intense than something that can just fix that bug and go into a patch release. Once I finish this I'll walk it back to something that can, maybe even just Greg's original fix + test. |
6b84cd1
to
0ab4094
Compare
0ab4094
to
cc03819
Compare
6d102f5
to
c35cccf
Compare
c35cccf
to
60d7fe7
Compare
Co-authored-by: MSP-Greg <MSP-Greg@users.noreply.github.com>
I think it's time for me to stop the spike for now and pull back a few of these things into master. |
As to the emoji I added, it reminded me of how "I want to fix a few issues" often turns into a lot more. At least that's been the case with me...
I think the current code only loads listeners in the parse code. ios is loaded in the add/inherit code. So, control url's aren't in listeners, and any sockets created via re-entrant calls in the add/inherit code are also not. I think... |
This commit extracts the `MiniSSL::Context` creation into its own `MiniSSL::ContextBuilder` class along the same lines as in [puma#1989]. This will allow us to reuse this code for adding SSL support to the control app (issue [puma#2015]). Since we will need the `MiniSSL` require and check in both places, I moved that into the `ContextBuilder` class as well. [puma#1989]: puma#1989 [puma#2015]: puma#2015
* Extract class for building SSL context This commit extracts the `MiniSSL::Context` creation into its own `MiniSSL::ContextBuilder` class along the same lines as in [#1989]. This will allow us to reuse this code for adding SSL support to the control app (issue [#2015]). Since we will need the `MiniSSL` require and check in both places, I moved that into the `ContextBuilder` class as well. [#1989]: #1989 [#2015]: #2015 * Add SSL support for the control app This starts to address [#2015]. I think we will need to add SSL support to the control cli as well. [#2015]: #2015
Closing as unmergeable, but I do want to revisit the ideas in here very soon. |
Doing an inheritance model could probably help me take a lot of the complexity out of Binder#parse and the various inherit_ and add_ listener methods...