Exception in NIO client when broker sends a large message over SSL

[dimas]

It looks like there is a bug in how SslEngineByteBufferInputStream class "reassembles" the stream.
The piece of code below either passes or fails depending on where I set breakpoint.

                int bytesRead = NioHelper.read(channel, cipherIn);
                if (bytesRead > 0) {
                    cipherIn.flip();
                } else {
                    bytesRead = NioHelper.retryRead(channel, cipherIn);
                    if(bytesRead <= 0) {
                        throw new IllegalStateException("Should be reading something from the network");
                    }
                }

If breakpoint is at NioHelper.read (so there is some time for data from the network be available and bytesRead be above zero) - it works.
However if breakpoint is set to the next line, data from the network is not yet available and else branch is executed. It eventually reads the data but never does cipherIn.flip(); so unlike the the "then" branch which exits with cipherIn(position=0, limit=4629) in my test, the "else" branch exits cipherIn(position=4629, limit=16921) - I guess it is still in "read" mode. That later fails on sslEngine.unwrap(). If I manually invoke cipherIn.flip() in the debugger, the packet seem to decode ok.

[acogoluegnes]

Thread on mailing list

[acogoluegnes]

@dimas I cannot reproduce locally, but your analysis looks correct. Can you try the latest 4.2.2 SNAPSHOT? Thanks.

[dimas]

It seem to work.
Regarding your PR - isn't it simpler to move cipherIn.flip(); outside of the if? Like this:

                int bytesRead = NioHelper.read(channel, cipherIn);
                if (bytesRead <= 0) {
                    bytesRead = NioHelper.retryRead(channel, cipherIn);
                    if(bytesRead <= 0) {
                        throw new IllegalStateException("Should be reading something from the network");
                    }
                }
                cipherIn.flip();

?
And re the test - my test involves a remote broker, I am not sure it is reproducible with localhost because timing is completely different in that case.
Also, my message is not large as yours - a JSON body of 4224 bytes is being sent + some headers (dunno, probably less than 256 bytes). Maybe for a better coverage it makes sense to run test multiple times with different message sizes.
Cheers

[michaelklishin]

Adding a few more tests with varying sizes is fine. Refactoring suggestions are best presented as pull requests :)