search

celluloid / reel

UNMAINTAINED: See celluloid/celluloid#779 - Celluloid::IO-powered web server
https://celluloid.io
MIT License
596 stars 87 forks source link

[SSL]Server.supervise_as broken after #116 #120

Closed digitalextremist closed 10 years ago

digitalextremist commented 10 years ago

As of #116 which works around #initialize, and implements a new #new, now #supervise_as functionality breaks, with ArguementError since #initialize accepts an already instantiated server, with a Hash of options. Previously, #initialize actually instantiate the server itself. Obviously the ability to create different kinds of servers is great, but now we cannot ( from what I can see ) easily instantiate a supervised Server or SSLServer actor.

This also impacts the usage examples, unless the existing interfaces to Server and SSLServer are retrofitted.

tarcieri commented 10 years ago

Ugh.

@digitalextremist thanks for finding this. I almost did another release. Perhaps we need some integration tests for interactions with the supervisor behavior.

@stouset I'd call this a release blocker. We need a more straightforward factoring of these classes for them to play nicely with the tricks that Celluloid is already using.

/cc @halorgium although I suppose you're probably on a plane to N-Zed

digitalextremist commented 10 years ago

@tarcieri this idea might be 'digging to China', but the way Server now looks, with all these patch-ins for different protocols, it seems like Server might need to become HTTPServer beside SSLServer beside UNIXServer, etc.

Again, 'digging to China' perhaps, and certainly changes the usage examples, but we've gone through a lot of changes to the point where maybe it is worth it? Maybe it can be digging to Tibet after the dust settles.

/cc @stouset, @halorgium

tarcieri commented 10 years ago

@digitalextremist I agree, we should make a Reel::HTTPServer class

digitalextremist commented 10 years ago

About done with the refactor of Server. Testing decentrality/reel ( feature branch ) in penultimatix/reel ( master )

digitalextremist commented 10 years ago

@stouset re:

If there's anything I can do to help finish this up, let me know. Given that my solution caused failures that weren't caught by the tests, we should probably have tests for that particular type of failure.

I've got the #supervise_as interface working in my code again. Yes, a test would be good, but could you test the UNIX server side? I am going to go through and change the usage examples and call it good.

Oh, also, the client certificate parameters need documentation. Will be back with updated usage shortly.

digitalextremist commented 10 years ago

All the usage example and documentation changes I could see are made. I see some tests failing, but at first glance that is because of SSLServer's client certificate stuff. @stouset I didn't change the tests at all, other than superficially to replace Server with HTTPServer as needed.

@tarcieri ready for run through and probably some kind of announcement that this is by no means backward compatible, unless we further modify Server to detect host and port being sent in and create HTTPServer rather than a server being sent in and just running with the premade HTTPServer, SSLServer or UNIXServer

Did this kinda fast with my stuff dying without #supervise_as but been wanting to contribute. In my use-case I have both SSLServer and HTTPServer running in the same application and I can verify it's back to its former state of those two instantiating well and playing nice ( with the same handler pool between them ).

Hopefully this is pretty clean and palatable for a pretty large change to the theory behind Reel::Server