Added SSL support - not sure if it is correct #17
Conversation
|
FYI - If you want to test it, this is my sample application: I'm pretty sure it doesn't work correctly (check out lib/helix/server.rb). When I try to do a large number of concurrent requests, I end up only being able to serve 2 req/s. I suspect I'm not using tasks correctly. |
2 similar comments
|
I will review this once we have something to rebase it on to. |
|
This is absolutely *not* how this should be implemented.
Monkeypatching core classes should be done extremely judiciously, and
without changing core behaviors. This is neither: it's modifying core
behaviors in a security-critical context.
I would suggest looking at (or perhaps even actually using) the
asynchronous SSL support in Socketry:
https://github.com/socketry/socketry/blob/master/lib/socketry/ssl/server.rb
On Mon, May 22, 2017 at 12:51 AM Samuel Williams ***@***.***> wrote:
I will review this once we have something to rebase it on to.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#17 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAADHZqTrAs_03LQamLh9sDoZn7yIL40ks5r8RRsgaJpZM4Nhxo3>
.
--
Tony Arcieri
|
|
@tarcieri Yeah, I agree, monkey patching the core is a bad idea. However, it's great that people are interested in the project and I'm sure we can use what has been done here as a base implementation for how to get things working. |
|
I agree, but I didn't see a way around it in terms of how async was implemented (i.e., it expected methods to exist on the SSLServer that did not). That is the main reason I asked for alternatives, so thanks for pointing out Socketry. The point is mostly moot at this point, considering the direction the async gem itself is going. I think it certainly makes more sense to base async's sockets on socketry (or use it directly). I actually looked at socketry briefly, but noticed in the README that it implemented synchronous semantics, and that it also lacked an accept_nonblock for SSL. I assume there is a reason for it (considering the core Ruby library does the same thing). I'll play with it and see if I can make it work with my project. I assume I can use socketry with nio4r in order to get back to async? That's at least what I'm planning to try out in the interim, unless I'm missing something. |
|
So the simple solution is that when you initialize OpenSSL server, you pass in a TCP server which implements non-blocking accept - I think that solves most of the issues, but I'll review it once I have time. |
|
Also you are right the synchronous IO model of socketry makes this a bit harder to use. However, the core async gem no longer cares about IO at all - it's a pure reactor. There is going to be a separate gem with all the Ruby-native IO wrappers, thats where this PR should land. In addition, we can explore supporting socketry on top of async, which should not only be feasible now, but a great idea - I'd like to keep options open - people can make their own choice about high level library. That's not the case with the async io wrappers which are coming up soon - they are opinionated and prefer blocks for resource management purposes. |
|
Can we please move this to https://github.com/socketry/async-io - thanks so much for your effort. |
I'm not sure if my hot-patching of OpenSSL::SSL::SSLServer is the correct way to do this, but it made it work in my example application. Open to other ideas of how to do it "correctly".