Support underflow handling without thread sleep

[YuryAndr]

Proposed Changes

Purpose of PR is to fix bug -#700

According to https://docs.oracle.com/javase/7/docs/technotes/guides/security/jsse/JSSERefGuide.html we should perform read next bytes of message when SSLEngineResult indicates BUFFER_UNDERFLOW state.

In versions before 5.*.* it was fixed using retries with Thread.sleep() https://github.com/rabbitmq/rabbitmq-java-client/blob/87aae532705490fa429afe34326a0ebbdd83e1d3/src/main/java/com/rabbitmq/client/impl/nio/NioHelper.java

But versions 5.*.* don't include this fix. Also It is not good idea to use Thread.sleep() on nio thred.

Current PR provides fetching chunks of bytes using standard NIO flow. SocketChannelFrameHandlerState provides isUnderflowHandlingEnabled attribute to track current state.

[acogoluegnes]

You beat me to it, I just pushed #702. You got the problem right, but I think #702 keeps the changes more local, WDYT?

[YuryAndr]

This PR takes into account cipherIn.clear() before next chunk reading at SocketChannelFrameHandlerStat. So there is no losses of previous chunks.

UPD It was realized that actually both PRs provide chunks handling without losses. But in case of #702 continueReading returns true due to the cipherIn.hasRemaining() == true. And it leads to reading from buffer in loop without checking selector.keys().

[acogoluegnes]

Nice contribution, thanks! Do you think it would be possible to get rid of the isUnderflowHandlingEnabled AtomicBoolean, keep the information in the frame builder, and expose with a package-protected method? The state could reset the flag with another package-protected method. This would avoid to use an atomic class (we don't need the guarantees it provides as this is all mono-threaded).

[acogoluegnes]

Can you elaborate on the need of these operations?

[YuryAndr]

cipherIn should aggregate chunks of bytes on each read. So we should not clear it before each chunk reading.

[YuryAndr]

Also in my implementation continueReading returns false when there is no data for next chunk. So there is no infinite loop around while(continueReading()) while data is not present in buffer. NIO thread can check selector state before trying to get bytes.

[acogoluegnes]

OK, removing some reading logic and putting it in the class that was already doing, nice.

[YuryAndr]

Do you think it would be possible to get rid of the isUnderflowHandlingEnabled AtomicBoolean

isUnderflowHandlingEnabled was converted to private attribute of SslEngineFrameBuilder

[michaelklishin]

@YuryAndr thank you for contributing. We will back port after testing but you are welcome to build this client from source and try it out on your actual workload one more time ;)

[acogoluegnes]

@YuryAndr 5.13.1-SNAPSHOT is available if you want to give it a try. I'll release 5.13.1 if it works for you.

[YuryAndr]

@acogoluegnes Thank you! It was verified over our setup.

Also it was found that PR had fixed another related issue.

It was noticed that error in BUFFER_UNDERFLOW state handling could lead to out of memory error.

Exception in thread "rabbitmq-nio" java.lang.OutOfMemoryError: Java heap space
	at com.rabbitmq.client.impl.nio.FrameBuilder.readFrame(FrameBuilder.java:96)
	at com.rabbitmq.client.impl.nio.NioLoop.run(NioLoop.java:156)
	at java.base/java.lang.Thread.run(Thread.java:829)

Because in case of 5.13.0 there is no BUFFER_UNDERFLOW state condition checking but only hasRemaining calls on buffers. So FrameBuilder tries to read random data from applicationBuffer and tries to create byte array of random size. For example - about 1.7 GB:

It was also verified that there are no such memory issues on 5.13.1-SNAPSHOT.

[YuryAndr]

@acogoluegnes Could you please also provide patch for 5.7.* version? Because we have modules with dependency on Spring 5.2.* version and spring-amqp 2.2.* https://github.com/spring-projects/spring-amqp/blob/2.2.x/build.gradle And it depends on 5.7.3. version of java client.

[michaelklishin]

We generally do not release patches for older minor series, in particular six series behind. @YuryAndr if you are willing to do all the backporting and testing work for 5.7.x, we can cut a new release.

[michaelklishin]

So yeah, in addition to backporting to 5.13.x-stable we'd have to backport to 5.7.x-stable and then produce a release.
I don't know if we have the pipelines to produce 5.7.x releases but I guess we can generate a one-off.

[YuryAndr]

Next PR provides cherry picked commits for 5.7.x #703

[acogoluegnes]

OK, I'll see what I can to release a 5.7.x, but it won't be before several weeks.