Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify Connection.upgradeFrom()/upgradeTo() #4971

Closed
sbordet opened this issue Jun 16, 2020 · 1 comment · Fixed by #5008
Closed

Simplify Connection.upgradeFrom()/upgradeTo() #4971

sbordet opened this issue Jun 16, 2020 · 1 comment · Fixed by #5008
Assignees

Comments

@sbordet
Copy link
Contributor

sbordet commented Jun 16, 2020

Jetty version
9.4.30

Description
Connection.upgradeFrom()/upgradeTo() has some wording in the javadocs regarding the ownership of the ByteBuffer and who is responsible to release it back to the pool.

However, this is often difficult to do because the upgrade code is invoked from places that cannot easily return the ByteBuffer to the pool, or must do so at a later time.

Likewise, upgradeTo() implementations may need to feed the ByteBuffer into already existing pooled buffers and are unsure of whether they own the ByteBuffer or not and whether they should return it to the pool.

The proposal is to make sure that all implementation of upgradeFrom() copy the remaining bytes into a "floating" ByteBuffer that does not come from the pool and hence does not need to be returned to it.

In this way, upgradeTo() implementations are free to use this "floating" ByteBuffer as they like: parse it immediately, or store it away to parse it later, or copy it into another pooled buffer, etc.

Care should be taken to make sure that the original network buffer is correctly released back to the pool after the remaining bytes have been copied into the "floating" ByteBuffer.

@sbordet sbordet assigned sbordet and unassigned lorban Jun 30, 2020
sbordet added a commit that referenced this issue Jun 30, 2020
Now the upgrade-from connection produces a "floating" buffer (not belonging to a pool), so that it can release the original buffer.

The upgrade-to connection is free to copy or store this "floating" buffer.

Updated javadocs and all implementations.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet sbordet linked a pull request Jun 30, 2020 that will close this issue
sbordet added a commit that referenced this issue Jun 30, 2020
Flip those buffers!

Signed-off-by: Simone Bordet <[email protected]>
sbordet added a commit that referenced this issue Jun 30, 2020
Fixed ConnectHandler by calling fillInterested() if the upgrade buffer is null.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet
Copy link
Contributor Author

sbordet commented Jul 3, 2020

After discussion during review, changing the current behavior in 9.4.x is a risk, as upgradeTo() implementation are supposed to pool the buffer they receive.
Pooling a "floating" buffer would be bad since it won't have the right capacity and may lead to BufferOverflowExceptions.

Moving this change to Jetty 10.

sbordet added a commit that referenced this issue Jul 6, 2020
Strengthened ByteBufferPool behavior when releasing non-pooled
ByteBuffers: the buffer is now discarded.

Signed-off-by: Simone Bordet <[email protected]>
sbordet added a commit that referenced this issue Jul 6, 2020
sbordet added a commit that referenced this issue Jul 6, 2020
Fixed javadocs.

Signed-off-by: Simone Bordet <[email protected]>
@sbordet sbordet closed this as completed in f2c6b67 Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants