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

Thought bubble to better deal with wrapping EOF and EMPTY chunks #9018

Closed
wants to merge 3 commits into from

Conversation

gregw
Copy link
Contributor

@gregw gregw commented Dec 7, 2022

I still think we can do better with retaining terminal chunks.

Since we have few implementations of Retainable that throw UnsupportedOperationException, I think it would be good to have an boolean isRetainable() method so we can know. Then Retainable.Wrapper and
a new Chunk.Wrapper can take action to use their own reference counter if they are passed a non retainable Retainable.

I think with something like this, we could always call release on passed chunks. If you want to call retain, then you just need to check that it is retainable... which you have to do now anyway!

This is far from complete or perfect.... I just think there might be something there to clean up just a bit.

thoughts?

@gregw gregw requested review from lorban and sbordet December 7, 2022 00:25
@gregw
Copy link
Contributor Author

gregw commented Dec 7, 2022

Even if we don't fully embrace this approach, I think asking a Retainable if it is really isRetainable makes a lot more sense to base retain/release logic on than asking a Chunk if it isTerminal.

The contract would then be that release must be called on any chunk that returns true from isRetainable. It would just be that EOF and EMPTY always return false. It may be that applications create other chunks that are not retainable.

@sbordet
Copy link
Contributor

sbordet commented Dec 7, 2022

@gregw, @lorban suggested an alternative solution, which is to modify boolean release() into void release() + onReleased().

In this way, chunk wrappers will still know exactly when they need to release, but the benefit is that retain()/release() can be implemented as no-ops rather than throwing.

Furthermore, we introduce isSpecial(), similar to isTerminal() but also true for Chunk.EMPTY, so that if a chunk is special, you may skip releasing it.
This is similar to your proposal (just opposite) in that you the contract is MUST call release() if not special.

@gregw
Copy link
Contributor Author

gregw commented Dec 7, 2022

@sbordet @lorban yeah this approach kind of looks interesting to start... but I couldn't work out how to actually use it easily where it counts.
Standing by to look at other ideas... but still happy to proceed with just better javadoc if we don't find something that really works well.

@gregw
Copy link
Contributor Author

gregw commented Dec 7, 2022

Closing this as I think the idea has been communicated... but I'm not sure it is actually workable.
More thought needed, although the current solution is OK if well documented.

@gregw gregw closed this Dec 7, 2022
@sbordet sbordet reopened this Dec 7, 2022
@sbordet
Copy link
Contributor

sbordet commented Dec 7, 2022

I'd like to keep this open because it is a valid concern that I'd like to examine deeper.

@joakime joakime added this to the 12.0.x milestone Dec 8, 2022
@gregw
Copy link
Contributor Author

gregw commented Dec 17, 2022

@sbordet has this been included in your other works etc. Can it now be closed?

@gregw
Copy link
Contributor Author

gregw commented Dec 20, 2022

@sbordet closing this as I think you have taken what you need into your branch

@gregw gregw closed this Dec 20, 2022
@gregw gregw deleted the jetty-12.0.x-isRetainable branch January 16, 2023 14:02
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 this pull request may close these issues.

3 participants