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.

Sign up for GitHub

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

Jump to bottom

Jetty 12: review/remove ByteBufferPool #9166

Closed
sbordet opened this issue Jan 12, 2023 · 3 comments · Fixed by #9195
Closed

Jetty 12: review/remove ByteBufferPool #9166

sbordet opened this issue Jan 12, 2023 · 3 comments · Fixed by #9195
Assignees
@sbordet
Labels
Bug For general bugs on Jetty side

Comments

@sbordet
Copy link
Contributor

sbordet commented Jan 12, 2023

Jetty version(s)
12

Description
The Jetty 12 code has now standardized to use RetainableByteBufferPool, so ByteBufferPool should be removed.

This would simplify a lot the codebase, and also clean up the Jetty component tree, because now the ByteBufferPools appear in the component tree, but not the RetainableByteBufferPools.

This is a prerequisite for #9148, because it would then allow to inject leak-tracking Pool instances.
@sbordet sbordet added the Bug For general bugs on Jetty side label Jan 12, 2023
@sbordet sbordet self-assigned this Jan 12, 2023
@gregw
Copy link
Contributor

gregw commented Jan 13, 2023

I'm just adding my voice of caution here, that we did try to do this before, but found a few cases that were just too hard to use the stricter Retainable contract for. I can't exactly recall what they were, so this time if we find them again and decide it can't be done, then we should document in the code what those cases are.

Note that I think we can change the API of RetainableByteBuffferPool to make it easier to leak track independently of removing the non retained pool. We can track leaks for just retainable usages.

1 similar comment
@gregw
Copy link
Contributor

gregw commented Jan 13, 2023

I'm just adding my voice of caution here, that we did try to do this before, but found a few cases that were just too hard to use the stricter Retainable contract for. I can't exactly recall what they were, so this time if we find them again and decide it can't be done, then we should document in the code what those cases are.

Note that I think we can change the API of RetainableByteBuffferPool to make it easier to leak track independently of removing the non retained pool. We can track leaks for just retainable usages.

sbordet added a commit that referenced this issue Jan 22, 2023
@sbordet
Fixes #9166 - Jetty 12: review/remove ByteBufferPool
35db099 
* Replaced usages of ByteBufferPool with RetainableByteBufferPool.
* Removed ByteBufferPool and related classes.

Signed-off-by: Simone Bordet <[email protected]>
sbordet added a commit that referenced this issue Jan 23, 2023
@sbordet
Fixes implicit narrowing conversion reported in #9166.
8076aa1 
Signed-off-by: Simone Bordet <[email protected]>
@joakime joakime linked a pull request Jan 24, 2023 that will close this issue
Fixes #9166 - Jetty 12: review/remove ByteBufferPool #9195
Merged
sbordet added a commit that referenced this issue Jan 27, 2023
@sbordet
Fixes #9166 - Jetty 12: review/remove ByteBufferPool (#9195)
ded18f5 
* Fixes #9166 - Jetty 12: review/remove ByteBufferPool

* Replaced usages of ByteBufferPool with RetainableByteBufferPool.
* Removed ByteBufferPool and related classes.
* Renamed oej.http2.frames.DataFrame.getData() -> getByteBuffer() for consistency.
* Removed Accumulator.acquire(), and updated code to use RetainableByteBufferPool.acquire() instead.
* Fixed HttpOutput callbacks to correctly call super.onCompleteSuccess() and super.onCompleteFailure().

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

sbordet commented Jan 27, 2023

Fixed by #9195.

@sbordet sbordet closed this as completed Jan 27, 2023
kohlschuetter added a commit to kohlschuetter/jetty.project that referenced this issue Feb 2, 2023
@kohlschuetter
Rename RetainableByteBufferPool to ByteBufferPool
8b2c0b8 
With issue jetty#9166, ByteBufferPool was removed and replaced by
RetainableByteBufferPool. Since ByteBufferPool was used by
AbstractConnector, this change broke backwards compatibility with
third-party connectors such as junixsocket-jetty.

Since there's no longer any other ByteBufferPool, rename the
RetainableByteBufferPool interface, and thereby not only reinstate
compatibility with existing third-party libraries but also save a few
keystrokes.

jetty#9284

Signed-off-by: Christian Kohlschütter <[email protected]>
@kohlschuetter kohlschuetter mentioned this issue Feb 2, 2023
Merged
kohlschuetter added a commit to kohlschuetter/jetty.project that referenced this issue Feb 3, 2023
@kohlschuetter
Rename RetainableByteBufferPool to ByteBufferPool
0a0fd8d 
With issue jetty#9166, ByteBufferPool was removed and replaced by
RetainableByteBufferPool. Since ByteBufferPool was used by
AbstractConnector, this change broke backwards compatibility with
third-party connectors such as junixsocket-jetty.

Since there's no longer any other ByteBufferPool, rename the
RetainableByteBufferPool interface, and thereby not only reinstate
compatibility with existing third-party libraries but also save a few
keystrokes.

jetty#9284

Signed-off-by: Christian Kohlschütter <[email protected]>
kohlschuetter added a commit to kohlschuetter/jetty.project that referenced this issue Feb 4, 2023
@kohlschuetter
Rename RetainableByteBufferPool to ByteBufferPool
b8ab66f 
With issue jetty#9166, ByteBufferPool was removed and replaced by
RetainableByteBufferPool. Since ByteBufferPool was used by
AbstractConnector, this change broke backwards compatibility with
third-party connectors such as junixsocket-jetty.

Since there's no longer any other ByteBufferPool, rename the
RetainableByteBufferPool interface, and thereby not only reinstate
compatibility with existing third-party libraries but also save a few
keystrokes.

jetty#9284

Signed-off-by: Christian Kohlschütter <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sbordet sbordet

Labels
Bug For general bugs on Jetty side
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fixes #9166 - Jetty 12: review/remove ByteBufferPool jetty/jetty.project
2 participants
@gregw @sbordet