geode-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF subversion and git services (Jira)" <>
Subject [jira] [Commented] (GEODE-6661) NioSslEngine has some problems in its ByteBuffer management
Date Wed, 06 Nov 2019 15:56:01 GMT


ASF subversion and git services commented on GEODE-6661:

Commit 4a69a7fea153ce4435e1c4caf777e21ab07a6128 in geode's branch refs/heads/develop from
Bruce Schuchardt
[;h=4a69a7f ]

Feature/geode 6661 (#4284)

* GEODE-6661 NioSslEngine has some problems in its ByteBuffer management

Reverting the change to use a temporary byte buffer for SSL handshakes.
At the end of a handshake the buffer may contain application data that
must be available for subsequent decryption.  In the case of TCPConduit
this is usually the "handshake" bytes transmitted for that package's
communications protocol.

Since we really need those bytes I've removed the option of expanding
the handshake buffer if it's smaller than the SSL session's required
packet size.  TCPConduit uses that figure to allocate the buffer so this
should be safe.  I've added a test for this.

* reverting investigative changes to test

* fix failing unit tests - adjusted buffer sizes

* reverted another set of investigative test changes

> NioSslEngine has some problems in its ByteBuffer management
> -----------------------------------------------------------
>                 Key: GEODE-6661
>                 URL:
>             Project: Geode
>          Issue Type: Bug
>          Components: messaging
>            Reporter: Darrel Schneider
>            Assignee: Bruce J Schuchardt
>            Priority: Major
>              Labels: performance
>          Time Spent: 0.5h
>  Remaining Estimate: 0h
> the NioSslEngine appears to have some problems with how it manages ByteBuffer instances,
>  # It has a "handshakeBuffer" instance variable and code that will conditionally create
it but higher level code always passes in a non-null inputBuffer. It should just be changed
to require an outside buffer. Also no need for an instance variable since "handshakeBuffer"
is only used by a single method. It can just be passed in to it.
>  #  The "myNetData" and "peerAppData" are both created as heap ByteBuffer instances
in the constructor. But if they ever need to be resized it does it by calling Buffers.expandWriteBufferIfNeeded
which will return the original heap ByteBuffer to the queue of buffers that should always
be direct byte buffers. And now the one used by NioSslEngine will be direct instead of heap.
This will also cause the stats that Buffers has to be wrong because we return a buffer to
it that we did not allocate from it.
>  # From a performance standpoint, we want to also have the buffer that we directly write
to a socket channel, or fill by reading from a socket channel, be a direct byte buffer. Other
byte buffers should not be direct. So normally the ByteBuffer we serialize an outgoing message
into is a direct ByteBuffer because it will be handed to the socket channel for the message
write. But in the case of the NioSslEngine we would want that first buffer to be a non-direct,
heap ByteBuffer. It ends up being passed to NioSslEngine.wrap which copies it into "myNetData".
The encrypted data in "myNetData" in turn is written to the socket channel so it is the one
that should be a direct ByteBuffer. For reading it is just the opposite. We read encrypted
data from the socket channel into what should be direct byte buffer (and it currently is because
it is allocated with Buffers). But then it is passed to NioSslEngine.unwrap which will copy
(and decrypt) what is in it into the "peerAppData". So "peerAppData" should be kept a heap
ByteBuffer. You can always get away with using either type of ByteBuffer. It is simply a performance
issue. What happens at a lower level in the jdk with a heap ByteBuffer being used with a socket
channel is that it eventually just copies it into a direct ByteBuffer and then uses it. That
extra copy can hurt performance and in the past we had trouble with the jdk caching of direct
ByteBuffers not reusing as well as ours and running out of direct memory.

This message was sent by Atlassian Jira

View raw message