-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
RetainableByteBuffer as mutable #11801
base: jetty-12.1.x
Are you sure you want to change the base?
Conversation
Small tweaks to the RBB API to make the concept more uniform throughout the codebase.
Small tweaks to the RBB API to make the concept more uniform throughout the codebase.
Too much noise in this PR. Will make another PR for it.
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ByteBufferPool.java
Outdated
Show resolved
Hide resolved
if (currentBuffer == null) | ||
throw new IllegalStateException(); | ||
|
||
if (currentBuffer.hasRemaining()) | ||
throw new IllegalStateException(); | ||
|
||
currentBuffer.release(); | ||
networkBuffer = new NetworkBuffer(); | ||
networkBuffer = bufferPool.acquire(bufferSize, isUseInputDirectByteBuffers()).asMutable(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks odd that ByteBufferPool.acquire()
returns non-mutable buffers.
jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/RetainableByteBuffer.java
Show resolved
Hide resolved
...ty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java
Outdated
Show resolved
Hide resolved
@@ -339,6 +334,15 @@ private void finish() | |||
} | |||
} | |||
|
|||
private void release() | |||
{ | |||
if (!released) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't accumulator.release()
be idempotent instead of using this released
flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The underlying reference counter class is not idempotent, as it throws if you release beyond 0.
Hence we can't just call accumulator.release()
without it throwing. This we need to boolean.
I think it is good that we check for over-releases
*/ | ||
default int getRetained() | ||
{ | ||
return -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0 sounds like a more sensible default, doesn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe. or throw Unsupported operation exception, as it should only be used be real retainables
* Creates a deep copy of this RetainableByteBuffer that is entirely independent | ||
* @return A copy of this RetainableByteBuffer | ||
*/ | ||
default RetainableByteBuffer copy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if that API isn't going to cause more harm than good as I fail to see when we'd like to make copies into newly allocated buffers, except in tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used in the take implementations.... which themselves are mostly used for tests..... but copy is something likely to be needed if usage of these APIs becomes more widespread, so I'd much rather we implement it and test it correctly.
If we don't use it, then it is not a problem. If we do, then it is needed.
I'd much rather have this API then see code doing BufferUtil.copy(rbb.getByteBuffer())
, which could result in multiple copies
|
||
/** | ||
* @return the capacity | ||
* @see #maxSize() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it the right moment to break backward compatibility and stop using int
sizing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach I've taken is to keep int usage for the APIs that are named the same as BB. So int capacity()
and int remaining()
. This means that existing code does not break.
But there are new equivalent methods that use long
, so long maxSize()
and long size()
are the equivalent of capacity
and remaining
.
if (!canRetain()) | ||
return new NonRetainableByteBuffer(slice); | ||
|
||
retain(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's good that you javadoc'ed that the originating RBB gets retained by slice(), but I remember having a lengthy discussion about slice() retaining or not with @sbordet, but I cannot remember the outcome, so we may need to revisit this later.
* @param last true if this is the last write. | ||
* @see org.eclipse.jetty.io.Content.Sink#write(boolean, ByteBuffer, Callback) | ||
*/ | ||
default void writeTo(Content.Sink sink, boolean last) throws IOException |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to expose blocking APIs here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is just a convenience wrapper of the async API. I found myself passing Callback.NOOP too often, which I think is dangerous as you can get code that looks like it works on simple tests, but fails if something blocks. Far better to provide the API so that short cuts are not taken elsewhere.
Every call to buffer.writeTo(sink, false, Callback.NOOP)
is a bug in waiting!
@lorban thanks for the review. I've answered a some of the questions and will work on some of the fixes/changes you suggest tomorrow. |
} | ||
|
||
@Override | ||
public Mutable slice(long length) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation of slice()
does not retain, unlike the version it overrides.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't need to. It retains the contained buffers and then returns an entirely new DynamicCapacity instance over those buffers. Thus it implements the contract. i.e it returns an independent set of pointers over shared data. When the nested buffers are consumed from either the original or the slice, they are released, so when both original and slice are consumed (or explicitly released), then so are the nested buffers
|
||
if (shouldAggregate(bytes, length) && append(bytes)) | ||
{ | ||
bytes.release(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks the Retainable
contract: it's up to the caller to release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. See javadoc for add. It explicitly says that responsibility is passed. This is how add differs from append.
return this; | ||
} | ||
|
||
_buffers.add(bytes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also breaks the Retainable
contract: you're keeping a ref to the RBB after returning, so you should retain it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, see javadoc for add
// generate first fragment with as HEADERS with possible priority | ||
generateHeader(accumulator, FrameType.HEADERS, length, flags, streamId); | ||
generatePriority(accumulator, priority); | ||
accumulator.add(hpack.slice(maxHeaderBlockFragment)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If hpack.slice()
retains (like the RBB impl does) then the RBB returned by slice()
should be released here. If it does not retains (like the RBB.DynamicCapacity impl does) then this code is fine... but it's breaking the Retainable
contract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The slice is retained, but then added to the accumulator, so the contract is that the accumulator take over responsibility for releasing the slice. If I had appended, then I would need to release here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gregw that is the opposite of the documented behavior that Retainable
s must have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbordet you explicitly asked me to provide the add semantic so we can "give" a RBB and not have to release it ourselves. The documented behaviour for Retainable is described as "typical" and as a "general rule", should I think is equivalent to a SHOULD rather than a MUST. The divergence from the norm is well documented in the signature of the add
method.
We could instead do:
accumulator.add(hpack.slice(maxHeaderBlockFragment)); | |
RetainableByteBuffer hpackSlice = hpack.slice(maxHeaderBlockFragment); | |
accumulator.append(hpackSlice); | |
hpackSlice.release(); |
But that is more verbose and means the reference counter is needlessly incremented and decremented, then we end up at the same place.
This is using exactly the same semantic as the existing org.eclipse.jetty.io.ByteBufferPool.Accumulator#append
, which also deviates from the "general rule", but without any javadoc saying so!
@lorban can you go through my responses and resolve any that you are satisfied with. I still have a few TODOs I'm working on as a result of your feedback.... |
@lorban Can you quickly go through and resolve any questions that you think are well enough answered? |
@gregw the only point of contention I see is the behavior of But this caught me by surprise, and I assume it could again in the future, more so if we multiply the exceptions. So I think there should be some hint in the method's name that it's not playing by the rules to suggest the caller of that API to go read the javadoc. How about renaming |
@lorban I too was caught by surprise that we already have some accumulators that have this semantic behind a method called
I'm open to change the name, but I don't like |
Opening this PR against 12.1.x rather than 12.0.x, replacing #11598
I'm just going to summarize the objectives of this PR: