Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Added setIssueHandshake() to enable auto-handshake #2388 #994

Merged
merged 2 commits into from Jan 4, 2013

Conversation

Projects
None yet
5 participants
Member

drewhk commented Jan 3, 2013

Attempt to fix the failed SSL tests.

Netty docs says that "You must make sure not to write a message while the handshake is in progress unless you are renegotiating. You will be notified by the ChannelFuture which is returned by the handshake() method when the handshake process succeeds or fails."

And "If isIssueHandshake() is false (default) you will need to take care of calling handshake() by your own. In most situations were SslHandler is used in 'client mode' you want to issue a handshake once the connection was established. if setIssueHandshake(boolean) is set to true you don't need to worry about this as the SslHandler will take care of it."

Member

drewhk commented Jan 3, 2013

Closing for now, as setIssueHandshake() does not solve the issue directly.

@drewhk drewhk closed this Jan 3, 2013

Member

drewhk commented Jan 3, 2013

Checked the netty code, and setIssueHandshake should do the trick:

    if (issueHandshake) {
        handshake().addListener(new ChannelFutureListener() {
            public void operationComplete(ChannelFuture future) throws Exception {
                if (!future.isSuccess()) {
                    Channels.fireExceptionCaught(future.getChannel(), future.getCause());
                } else {
                    ctx.sendUpstream(e);
                }

            }
        });

Writes should be buffered still, but that is handled by the remoting code. We should give it a try I think.

@drewhk drewhk reopened this Jan 3, 2013

Owner

viktorklang commented Jan 3, 2013

No, you can't assume that !isSuccess == failure, you're forgetting cancellation

Member

drewhk commented Jan 3, 2013

Hmm, it's their code, so I can't fix it... Then I have to do this manually.

Member

drewhk commented Jan 3, 2013

If they don't cancel the future, can isCancelled be true?

Owner

viktorklang commented Jan 3, 2013

How do you know if they're ever going to cancel it?

Member

drewhk commented Jan 3, 2013

Then it's their bug :) The code I pasted is from SslHandler.

@patriknw patriknw commented on the diff Jan 3, 2013

...cala/akka/remote/transport/netty/NettyTransport.scala
@@ -230,10 +231,17 @@ class NettyTransport(private val settings: NettyTransportSettings, private val s
}
private val associationListenerPromise: Promise[AssociationEventListener] = Promise()
+
+ private def sslHandler(isClient: Boolean): SslHandler = {
+ val handler = NettySSLSupport(settings.SslSettings.get, log, isClient)
+ handler.setIssueHandshake(true)
@patriknw

patriknw Jan 3, 2013

Owner

why is this not part of NettySSLSupport.apply?

@drewhk

drewhk Jan 3, 2013

Member

I was thinking about that, too. I cannot do that however, until I remove the old remoting, as it handles this by hand.

@rkuhn

rkuhn Jan 3, 2013

Collaborator

in that case please add a ticket

@drewhk

drewhk Jan 3, 2013

Member

Let's agree first on if we want to use the build-in netty stuff (setIssueHandshake), or go with the manual way (calling handshake and waiting on completion). I think the former is cleaner if it works (which I think it will).

@viktorklang

viktorklang Jan 3, 2013

Owner

If setIssueHandshake works, then we should use it, since that's what it's for.

@patriknw patriknw and 1 other commented on an outdated diff Jan 3, 2013

...cala/akka/remote/transport/netty/NettyTransport.scala
private val serverPipelineFactory: ChannelPipelineFactory = new ChannelPipelineFactory {
override def getPipeline: ChannelPipeline = {
val pipeline = newPipeline
- if (EnableSsl) pipeline.addFirst("SslHandler", NettySSLSupport(settings.SslSettings.get, log, false))
+ if (EnableSsl) pipeline.addFirst("SslHandler", sslHandler(false))
@patriknw

patriknw Jan 3, 2013

Owner

named parameter would make it more readable isClient = false

@rkuhn

rkuhn Jan 3, 2013

Collaborator

+1

Owner

patriknw commented Jan 3, 2013

apart from the style comments, LGTM

Collaborator

rkuhn commented Jan 3, 2013

LGTM, in the @patriknw sense

Collaborator

rkuhn commented Jan 3, 2013

LGTM

PLS REBUILD ALL

Owner

viktorklang commented Jan 3, 2013

Is the kitteh dead or something?

Collaborator

akka-ci commented Jan 3, 2013

Started jenkins job akka-pr-validator at https://jenkins.akka.io/job/akka-pr-validator/269/

Collaborator

akka-ci commented Jan 3, 2013

jenkins job akka-pr-validator: Success - https://jenkins.akka.io/job/akka-pr-validator/269/

@drewhk drewhk added a commit that referenced this pull request Jan 4, 2013

@drewhk drewhk Merge pull request #994 from drewhk/wip-2833-ssl-remoing-failed-drewhk
Added setIssueHandshake() to enable auto-handshake #2388
6924d53

@drewhk drewhk merged commit 6924d53 into akka:master Jan 4, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment