Skip to content
This repository has been archived by the owner on Dec 7, 2018. It is now read-only.

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

Closed
digitalextremist opened this issue Nov 13, 2013 · 6 comments · Fixed by #121
Closed

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

digitalextremist opened this issue Nov 13, 2013 · 6 comments · Fixed by #121

Comments

@digitalextremist
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

@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
Copy link
Member

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

@digitalextremist
Copy link
Member Author

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

@digitalextremist
Copy link
Member Author

@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
Copy link
Member Author

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants