SSL support #6

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
3 participants

bbanko commented Feb 14, 2012

Joe, please can you review the work. More or less this should be near finished SSL support. The SSL is a wide subject, so I may miss something. What I'm not happy with, is the problem with how netty exception is handled upon unsuccessful bind at the server side. You can see that problem in the unit test UntrustedClientSSL() where bind() hangs out. I've tried to debug, but I would need to much time. You will probably see the problem sooner. It could be a general problem with how such exceptions are handled, or I failed to properly configure the tests.

Contributor

jjlauer commented Feb 18, 2012

Thanks for doing a good job on a first draft impl with good coverage on unit tests. The unit test that continues to hang is definitely a bug with Netty. Netty fails to notify its own "handshakeFuture" when a channel is closed during the SSL handshake. The smpp lib relied on a couple "future.wait()" calls -- assuming netty was reliable. The indefinite wait() calls basically trigger deadlock -- since netty never notifies the future.

I confirmed even the latest version of Netty (3.3.1) has the same issue.

I have a very good workaround finished for the SSL handshake. Basically, a simple loop that uses future.wait(timeout) calls along with checking if the channel is still connected before trying to wait again. Worst case is that an SSL handshake failure takes up to 500 ms to detect. Acceptable on a connection failure.

I plan on removing all other future.wait() calls to be safe. There are a few used in both sending requests and responses on the wire. I think we're in good shape to get this accepted into the master branch within the next 2 weeks -- it won't be very different than your version -- so feel free to use your own branch until I get this out in an official release soon.

bbanko commented Feb 20, 2012

Good news. I'm looking forward for a next release so we can use the official version. Thank you.

Contributor

xgp commented Apr 11, 2013

An updated version of SSL support was merged into master. It uses some parts of these suggested changes.

@xgp xgp closed this Apr 11, 2013

aspan pushed a commit to aspan/cloudhopper-smpp that referenced this pull request Mar 25, 2016

Merge pull request #6 from JChrist/netty4
Netty4 merged with master (plus updates)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment