Skip to content
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

Implemented SSL (wss://) support #64

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vbguyny
Copy link

@vbguyny vbguyny commented Feb 8, 2013

Tested with Chrome 24, Firefox 18, and Safari 5.1.7
Confirmed backwards compatibility of non-SSL (ws://) with same browsers

@vbguyny vbguyny mentioned this pull request Feb 8, 2013
@jhaygood86
Copy link
Contributor

You are using Write and Read. You might be able to get it to get similar performance using WriteAsync and ReadAsync (which should call the underlying Socket's SendAsync and ReceiveAsync methods). You need to use relatively static threads in order for those to work correctly (IOCP threads fail if the thread they were started from dies). Writing async code is pretty hard as well. However, it doesn't look like Stream supports the high performance async I/O operations directly however. An easy way to test is to create a websocket client that creates 10,000 simultaneous connections. The original should support it, your modified one most likely won't, since its doing blocking reads and blocking writes.

For reference, before commit 3b49d0c, master had a similar problem (it was creating one system thread per connection, instead of using one thread per CPU and using IOCP -- though they did use the IOCP methods..)

@vbguyny
Copy link
Author

vbguyny commented Feb 10, 2013

I first tried to do the WriteAsync and ReadAsync methods however WriteAsync would fail with an error saying that the function was already being performed. After some research, the work around was to use the blocking methods in separate worker threads. The program will create a new thread for each client so I don't see how it could cause the entire program block (and I did test for this).

@jhaygood86
Copy link
Contributor

It’s creating the thread for each client that causes the performance issues. Windows doesn’t like having lots threads of open (a few hundred will be slow, a few thousand is impossible). Having 1 thread handle 15,000 connections (which is what using IOCP does) is A LOT better than using 15,000 threads for 15,000 connections – Windows will basically die under the thread switches.

From: vbguyny [mailto:notifications@github.com]
Sent: Sunday, February 10, 2013 2:58 PM
To: Olivine-Labs/Alchemy-Websockets
Cc: Justin Haygood
Subject: Re: [Alchemy-Websockets] Implemented SSL (wss://) support (#64)

I first tried to do the WriteAsync and ReadAsync methods however WriteAsync would fail with an error saying that the function was already being performed. After some research, the work around was to use the blocking methods in separate worker threads. The program will create a new thread for each client so I don't see how it could cause the entire program block (and I did test for this).


Reply to this email directly or view it on GitHubhttps://github.com//pull/64#issuecomment-13359804..

@vbguyny
Copy link
Author

vbguyny commented Feb 10, 2013

Agreed. However the SSL Stream doesn't appear to support what you are suggesting. The WriteAsync doesn't work as per the current implementation.

@jhaygood86
Copy link
Contributor

I’m not a maintainer, so I can’t for sure say to not accept it, but I recommend it until the problem is taken care. One thread per connection is not scalable except for low volume requests. While my employer is using a fork (not much different than origin master though), especially since setting up an SSL termination node in front of Alchemy is so easy (see stunnel, haproxy, etc.. for instance, and major cloud vendors both have load balancers that you can use with SSL termination as a feature).

Current State:

· No native SSL support, but it’s possible to get SSL terminated and have Alchemy talk with the SSL termination node unencrypted

· Handles 10k+ (and even 100k+ on suitable hardware) simultaneous connections on a single node without even blinking. On idle, CPU is 0%

With This Patch:

· Native SSL support

· Handles a few hundred simultaneous connections, in the right conditions You might be able to do over 1000, but 10,000 is going to be impossible when running on Windows at least. On idle with that many connections (assuming you didn’t crash yet), CPU is still high due to the constant thread switches.

From: vbguyny [mailto:notifications@github.com]
Sent: Sunday, February 10, 2013 3:30 PM
To: Olivine-Labs/Alchemy-Websockets
Cc: Justin Haygood
Subject: Re: [Alchemy-Websockets] Implemented SSL (wss://) support (#64)

Agreed. However the SSL Stream doesn't appear to support what you are suggesting. The WriteAsync doesn't work as per the current implementation.


Reply to this email directly or view it on GitHubhttps://github.com//pull/64#issuecomment-13360413..

@vbguyny
Copy link
Author

vbguyny commented Feb 10, 2013

OK. I can change the code so that non-SSL works the way that it always had and SSL works this new way.

@ajacksified
Copy link
Member

Thanks for the work you've put into this! For some of the reasons stated above, we think that it's much better to handle SSL termination upstream (as mentioned by @jhaygood86) by something better suited to handling SSL so that Alchemy can continue being very performant with lightweight socket connections.

If there's a case where you can't run with SSL termination, I'd be interested in hearing your requirements.

@logicethos
Copy link

I'm a long time user of Alchemy. For a new project I was going to switch away to something else, as all my customers expect everything to be SSL now. The NSA have something to do with it. :)

fleck is not suitable for what I want. This will do, thanks. SSL should be in the new branch IMO.

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

Successfully merging this pull request may close these issues.

5 participants