Fixes retainability of special Chunks #9073

sbordet · 2022-12-19T22:24:20Z

Alternative to #9018.

Restored Jetty 11's AsyncContentProducer and related classes.
Introduced Retainable.canRetain().
Removed Chunk.isTerminal() and replaced it with alternative method calls.

Signed-off-by: Simone Bordet simone.bordet@gmail.com

gregw

Just some comments on my first look

jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/ResponseNotifier.java

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java

...t/src/main/java/org/eclipse/jetty/http2/client/transport/internal/HttpReceiverOverHTTP2.java

...t/src/main/java/org/eclipse/jetty/http3/client/transport/internal/HttpReceiverOverHTTP3.java

...-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java

lorban

I'm surprised by the fact that Chunk.from now calls retain(), and it doesn't help readability and possibly hurts perf.

jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/ResponseNotifier.java

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Retainable.java

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java

* Restored Jetty 11's AsyncContentProducer and related classes in jetty-ee9-nested module (src and test). * Introduced Retainable.canRetain(). * Removed Chunk.isTerminal() and replaced it with alternative method calls. * Clarified AsyncContent.write() in case of Chunk.Error. * Removed AsyncContent.write(Chunk, Callback) because it was making the API confusing. For example, AsyncContent.close() clashing with write(EOF, NOOP), or AsyncContent.fail(x) clashing with write(Chunk.Error, NOOP), etc. * Improved usage of Chunk.from(..., Retainable). * Improved usage of Chunk.slice(). Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

gregw · 2023-01-04T06:18:35Z

@sbordet ARGH! why force pushed???? We lose the context of all the conversations so re-review is so much harder! The PR is going to be squashed anyway, so forced pushes should be avoided!

gregw

Not a full review... just some questions???

jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/ResponseNotifier.java

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java

gregw · 2023-01-04T06:30:43Z

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java

+            }
+            else
+            {
+                return isLast() ? this : EMPTY;


Suggested change

return isLast() ? this : EMPTY;

return isLast() ? EOF : EMPTY;

Nope, this can be an Error chunk.

as above: return EOF here and extend the method in Error to return this

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java

gregw

As I look at this PR more, I'm more OK with it. Specifically I'm OK with the from method not doing a retain. But I think the slice method should.
I also think we are duplicating the hasRemaining checks too often.

But these are not almost down to niggle status for me. (other than the this return).

gregw · 2023-01-09T07:17:01Z

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java

+            if (hasRemaining())
+                return from(getByteBuffer().slice(), isLast(), this);
+            else
+                return isLast() ? this : EMPTY;


The from method already has logic to check for remaining and do EOF/EMPTY return, so we should not duplicate here. We also should retain here as a slice results in 2 chunks pointing to the same buffer. The only place this method is currently called from is ResponseNotifier#onChunk and it does the retain anyway!

Suggested change

if (hasRemaining())

return from(getByteBuffer().slice(), isLast(), this);

else

return isLast() ? this : EMPTY;

retain();

return from(getByteBuffer().slice(), isLast(), this);

The point of not retaining in slice() is that it does not know how the returned slice is used.
Even if the (currently) only call stores the slice away for later use (and so has to retain), in future that may not be true, and we only change the code that actually uses the slice because that is what would have changed.

gregw · 2023-01-09T07:18:02Z

jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/ResponseNotifier.java

+                    // Retain the slice because it is stored for later reads.
+                    if (slice.canRetain())
+                        slice.retain();


The retain should be done by slice. If the chunk is not retainable, should it be duplicated here?

The retain should not be done by slice(), because slice() does not know how the slice is used (whether consumed immediately or stored away for later use).
The retain is done here exactly because the sliced chunk is stored for later use, like the comment says.

If a chunk is created, then it needs to be released. Methods read, from and slice create a new chunk, so a release should be needed, so slice needs to retain. Only if you are passed a chunk as an arg do you not have to release it. Instead you have to retain it if you want to keep it (and then call release).

@sbordet Even in the case where a slice is used synchronously (i.e Multi-part), I'm cautious about not retaining or even passing a reference to the original retainable. The chunk is passed to application code, which can be wrong. We used to require a release, but now we no longer do. If the application erroneously does release, then they will release the original buffer back into the pool, but the MultiPart parser will keep parsing. Thus I'm thinking that if we are slicing to get a chunk that we protect from evil code moving pointers where they should not, then we should also protect against evil/bad code going negative with their reference count. I.e the application should never be able to cause the parser to release the buffer it is currently parsing.

gregw · 2023-01-09T07:46:25Z

@sbordet In multipart line 1206 there is existing code that does:

                Content.Chunk content = chunk.slice(buffer.position(), sliceLimit, true);
                buffer.position(buffer.position() + boundaryOffset + boundaryFinder.getLength());
                notifyPartContent(content);
                notifyPartEnd();

Currently the slice call does not retain, so the content chunk passed to the notifyPartContent method, which call the listener, whose javadoc says it must release the chunk.... but since slice didn't retain, then when the listener releases it will release the original chunk.

I'm back at my desk tomorrow. Let's schedule a time to walk through this code together.

sbordet · 2023-01-09T09:14:11Z

@gregw you are right that the javadoc was still documenting the old behavior. I fixed the javadoc to document the new behavior.

* Using from() in MultiPart, rather than duplicating code. * Made Retainable.canRetain() non-default, and provided implementations explicitly. * Fixed MultiPart.Parser.Listener.onPartContent() javadocs. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

…al'. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

…al'. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

* Renamed non-retaining Chunk.from() to Chunk.asChunk(). * Removed Chunk.slice() methods, inlining them where necessary. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

gregw

Other than a niggle and CI failure. OK!

gregw · 2023-01-10T20:40:42Z

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Retainable.java

+     * <p>Returns whether this resource can be retained, that is whether {@link #retain()}
+     * can be called safely.</p>
+     * <p>Implementations may decide that special resources are not retainable (for example,
+     * {@code static} constants) so calling {@link #retain()} is not safe because it may throw.</p>


Can we say that calling release on non-retainable Retainable is OK.

…al'. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

lorban · 2023-01-11T09:18:05Z

...y-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpReceiverOverHTTP.java

@@ -450,9 +450,10 @@ public boolean content(ByteBuffer buffer)
        if (chunk != null)
            throw new IllegalStateException("Content generated with unconsumed content left");

-        RetainableByteBuffer networkBuffer = this.networkBuffer;
+        // Retain the chunk because it is stored for later use.


--- // Retain the chunk because it is stored for later use. +++ // Retain the networkBuffer because it is stored as a chunk for later use.

jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/Response.java

...t/src/main/java/org/eclipse/jetty/http3/client/transport/internal/HttpReceiverOverHTTP3.java

lorban · 2023-01-11T09:29:54Z

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/AsyncContent.java

-        // byte, meaning it is either EOF or EMPTY. The callback is succeeded
-        // once and only once, but that happens either during read() if the
-        // byte buffer is empty or during Chunk.release() if it contains at
-        // least one byte.
        Content.Chunk chunk;
        if (byteBuffer.hasRemaining())


Isn't Content.Chunk.from already doing that byteBuffer.hasRemaining() check?

lorban · 2023-01-11T09:31:24Z

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/AsyncContent.java

@@ -77,53 +68,19 @@ public void write(boolean last, ByteBuffer byteBuffer, Callback callback)
        offer(chunk, callback);
    }


Technically, the chunk should be released after offer returns. We can avoid doing it because offer is private and we know it always retains to store the chunk for later and we save a useless retain/release pair.

But that reason probably is worth commenting.

…tainable instance to make sure it is released. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

gregw · 2023-01-11T22:51:23Z

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/content/AsyncContent.java

        }
-        if (!current.chunk().hasRemaining())
+        if (!current.chunk().canRetain())


This is the only call to canRetain that has any substantive behavior associated with it. All other calls are in the form:

if (chunk.canRetain()) chunk.retain();

and there is never an else clause or change to the following logic. So that raises the question: why don't we just make retain a noop for chunks that are not cannot be retained? The answer is that we need to implement this very specific if statement in this and only this method!!!!

So at the very least, we should put a great big comment here saying how significant this if statement is.

Also the comment at line 73 above still talks about "terminal". So it needs to be updated and this mechanism better documented.

I think think we need to take one final step back and decide if canRetain really is the right API? Perhaps we just need an isConstant that is used only at this location?

More over, we are only using canRetain to pass information between methods of this class, which already has a queue of records. We can just put that information (to succeed when read) into the object that we queue.

Standby for a PR to this PR....

See #9159 that shows t hat canRetain is no longer needed

I've updated #9159 as per our hangout discussion. I'm OK with it being merged here or this being merged first and mine second.

lorban

LGTM

… retain/release semantic. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

@disabled

…x-document-modules * upstream/jetty-12.0.x: Issue jetty#9167 - making assumption in flaky test jetty 12.0.x cleanup duplicate osgi pom metadata (jetty#9093) Jetty 12 - Add tests in util/resource for alternate FileSystem implementations (jetty#9149) Cleanup non-retainable `Retainable`s (jetty#9159) Fixes retainability of special Chunks (jetty#9073) TCK: Dispatch forward and includes attributes do not meet the spec (jetty#9074) re-enable h3 tests (jetty#8773) More fundamental test case Reorganization of jetty-client classes. (jetty#9127) Removing @disabled from SslUploadTest Removing @disabled from jetty-start jetty#9134 added test ee10: DefaultServlet: Replace checks for isStreaming() by !isWriting() jetty#9078 make HttpContent.getByteBuffer() implementations return new ByteBuffer instances and document that contract Fixes jetty#9141 - Thread-safe Content.Chunk#slice (jetty#9142) Remove `@Disabled` from `jetty-jmx` (jetty#9143) Bump maven.version from 3.8.6 to 3.8.7 Bump maven.version from 3.8.6 to 3.8.7

@disabled

… into jetty-12.0.x-documentation-lowercase-jetty_home-attribute * 'jetty-12.0.x' of https://github.com/eclipse/jetty.project: Issue jetty#9167 - making assumption in flaky test jetty 12.0.x cleanup duplicate osgi pom metadata (jetty#9093) Jetty 12 - Add tests in util/resource for alternate FileSystem implementations (jetty#9149) Cleanup non-retainable `Retainable`s (jetty#9159) Fixes retainability of special Chunks (jetty#9073) TCK: Dispatch forward and includes attributes do not meet the spec (jetty#9074) re-enable h3 tests (jetty#8773) More fundamental test case Reorganization of jetty-client classes. (jetty#9127) Removing @disabled from SslUploadTest Removing @disabled from jetty-start Bump maven.version from 3.8.6 to 3.8.7 Bump maven.version from 3.8.6 to 3.8.7

sbordet requested review from gregw and lorban December 19, 2022 22:24

gregw reviewed Dec 20, 2022

View reviewed changes

lorban requested changes Dec 20, 2022

View reviewed changes

sbordet linked an issue Dec 25, 2022 that may be closed by this pull request

Content.Chunk.isTerminal() cannot discriminate EOF from chunks containing a pooled empty buffer #8993

Closed

sbordet force-pushed the fix/jetty-12-remove-chunk-isTerminal branch from c1a2c36 to 9be8c1a Compare January 1, 2023 19:11

joakime added the Jetty 12 label Jan 2, 2023

joakime added this to the 12.0.x milestone Jan 2, 2023

sbordet force-pushed the fix/jetty-12-remove-chunk-isTerminal branch from 9be8c1a to 1ffd67b Compare January 3, 2023 17:57

sbordet marked this pull request as ready for review January 3, 2023 18:02

sbordet requested review from lorban and gregw January 3, 2023 18:02

gregw reviewed Jan 4, 2023

View reviewed changes

sbordet requested review from gregw and joakime January 4, 2023 09:52

gregw requested changes Jan 9, 2023

View reviewed changes

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java Outdated Show resolved Hide resolved

jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/Content.java Outdated Show resolved Hide resolved

gregw requested changes Jan 9, 2023

View reviewed changes

sbordet added 5 commits January 9, 2023 10:15

Updates from review.

9de4a5b

* Using from() in MultiPart, rather than duplicating code. * Made Retainable.canRetain() non-default, and provided implementations explicitly. * Fixed MultiPart.Parser.Listener.onPartContent() javadocs. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Merged branch 'jetty-12.0.x' into 'fix/jetty-12-remove-chunk-isTermin…

e478a15

…al'. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Restored correct code.

c0ec040

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Merged branch 'jetty-12.0.x' into 'fix/jetty-12-remove-chunk-isTermin…

917807c

…al'. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Updates after review.

73e6e34

* Renamed non-retaining Chunk.from() to Chunk.asChunk(). * Removed Chunk.slice() methods, inlining them where necessary. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet requested a review from gregw January 10, 2023 09:18

gregw approved these changes Jan 10, 2023

View reviewed changes

sbordet added 2 commits January 11, 2023 10:14

Merged branch 'jetty-12.0.x' into 'fix/jetty-12-remove-chunk-isTermin…

1438b32

…al'. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Fixed test failures.

55a24bb

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

lorban requested changes Jan 11, 2023

View reviewed changes

sbordet added 2 commits January 11, 2023 16:54

Carefully reviewed all usages of read()-like methods that return a Re…

7765114

…tainable instance to make sure it is released. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Fixed test.

b88f256

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet requested review from lorban and gregw January 11, 2023 21:34

gregw requested changes Jan 11, 2023

View reviewed changes

gregw approved these changes Jan 12, 2023

View reviewed changes

lorban approved these changes Jan 12, 2023

View reviewed changes

sbordet added 2 commits January 12, 2023 14:17

Updated HTTP/2 and HTTP/3 usages of Stream.Data to follow the correct…

b9e21c2

… retain/release semantic. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

Fixed retain count for HTTP2Session.StreamData.

6934d83

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>

sbordet removed this from the 12.0.x milestone Jan 12, 2023

sbordet merged commit 46355c6 into jetty-12.0.x Jan 13, 2023

sbordet deleted the fix/jetty-12-remove-chunk-isTerminal branch January 13, 2023 00:11

gregw mentioned this pull request Jan 13, 2023

Jetty 12 simplify Retainable.canRetain usage #9159

Merged

sbordet mentioned this pull request Jan 13, 2023

Content.Chunk.isTerminal() cannot discriminate EOF from chunks containing a pooled empty buffer #8993

Closed

sbordet mentioned this pull request Jan 27, 2023

Jetty 12 - Fix ChunksContentSource #9212

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes retainability of special Chunks #9073

Fixes retainability of special Chunks #9073

sbordet commented Dec 19, 2022

gregw left a comment

lorban left a comment

gregw commented Jan 4, 2023

gregw left a comment

gregw Jan 4, 2023

sbordet Jan 4, 2023

gregw Jan 9, 2023

gregw left a comment

gregw Jan 9, 2023

sbordet Jan 9, 2023

gregw Jan 9, 2023

sbordet Jan 9, 2023

gregw Jan 9, 2023

gregw Jan 9, 2023

gregw commented Jan 9, 2023

sbordet commented Jan 9, 2023

gregw left a comment

gregw Jan 10, 2023

lorban Jan 11, 2023

lorban Jan 11, 2023

lorban Jan 11, 2023

gregw Jan 11, 2023

gregw Jan 12, 2023

gregw Jan 12, 2023

gregw Jan 12, 2023

lorban left a comment

	return isLast() ? this : EMPTY;
	return isLast() ? EOF : EMPTY;

		@@ -77,53 +68,19 @@ public void write(boolean last, ByteBuffer byteBuffer, Callback callback)
		offer(chunk, callback);
		}

Fixes retainability of special Chunks #9073

Fixes retainability of special Chunks #9073

Conversation

sbordet commented Dec 19, 2022

gregw left a comment

Choose a reason for hiding this comment

lorban left a comment

Choose a reason for hiding this comment

gregw commented Jan 4, 2023

gregw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregw commented Jan 9, 2023

sbordet commented Jan 9, 2023

gregw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorban left a comment

Choose a reason for hiding this comment