Refactor Reel::Server, refactor/retrofit/abandon SocketMixin #121
Conversation
…for SocketMixin )
|
Shouldn't the class names be updated to reflect their new location? e.g., |
| optimize_socket @tcpserver = Celluloid::IO::TCPServer.new(host, port) | ||
|
|
||
| server = Celluloid::IO::SSLServer.new(@tcpserver, ssl_context) | ||
| options.merge!({ :host => host, :port => port }) |
There was a problem hiding this comment.
Is this necessary if the TCPServer has already been established on the host/port?
There was a problem hiding this comment.
That is where the TCPServer is created though, which is then wrapped in IO::SSLServer and then the wrapped server is sent to Reel::Server at super(server, options, &callback)
There was a problem hiding this comment.
Right, I mean the merging of host and port into the options hash.
There was a problem hiding this comment.
@stouset I do that so the Server can be asked for what port it is on, and what host it is listening on. It is not necessary, per se, but I saved @options in Server and you'll see in UNIXServer I also save @options[:socket_path]
In what I've seen, implementations sometimes need to know the underlying options the server is configured for to know how to setup environments properly request by request, or connection by connection.
|
Didn't want to assume @tarcieri would want the |
|
It looks like the SSLServer errors are due to the fact that the SSLServer no longer serves HTTP, but I'm not absolutely certain yet. |
| # TCP_CORK: TODO: tersely describe | ||
| # SO_REUSEADDR: TODO: tersely describe | ||
|
|
||
| if RUBY_PLATFORM =~ /linux/ |
There was a problem hiding this comment.
Can you leave this out for the time being?
|
In general I think it'd be good if you left the SocketMixin refactoring out of this PR and focused just on changing the class names |
| end | ||
|
|
||
| end | ||
| end |
There was a problem hiding this comment.
HTTPServer looks like it can no longer be composed with SSLServer, which was the whole point of the original approach. :(
|
@tarcieri about to make all the requested changes in the next few minutes. Can we settle on keeping My justification is that we have two Starting on those changes. |
|
Yeah, I did just plain |
|
Alright, the moment of truth... all tests running, with both |
|
|
|
Coming back to pull the trigger on this merge when you feel comfortable with the PR @tarcieri. |
|
@tarcieri you may want to check And I will see if |
|
...and |
|
@digitalextremist looks like you can probably just use "rbx". Surprised it's failing with rubysl in the Gemfile I'd say let's just move rbx to allowed failures for now (and leave out rbx-2 since it's the same thing) |
|
@tarcieri, after what we've just gone through and seen, this commit will pass. Newline added, and failure of |
|
All green with the one allowed red. |
|
Last things that ought to be looked at later and can move to separate issues, which are @stouset territory:
All set to merge, and move those two things into their own issues. Update: The issues were created, and added above. |
|
Come to think of it @tarcieri, since this affects a lot of past code out there in the wild and will break that code, #126 ought to happen as simultaneously as possible. I am open to scheduling that at a point you are comfortable with it, volunteering to do the Wiki changes and then immediately come and merge this in once the documentation isn't one huge o_0 field of uhhh? And I could also write a post on the mailinglist about this unless someone else would be better. |
|
@digitalextremist this is definitely a breaking change that needs to be well-advertised, and I agree about #126 |
|
Sweet, thank you! |
|
This is good to merge |
Refactor Reel::Server, refactor/retrofit/abandon SocketMixin, depending on #126 immediately hereafter.
|
🎉 🎉 🎉 |
👍
|
😃 I'm stupid happy. Wiki fully updated. Only thing left on #126 is the mailing list post. Can you check over the Wiki when you get a chance? |
|
Sure, will probably be some time tomorrow |
|
Thanks again! |
|
My pleasure 👍 I am honored to step in, stick around, and see |
|
Nice work! Now we need celluloid 1.0! |
Closes #114, Closes #119, Closes #120