Make sure ContentSourceTransformer.read() calls release() on fail() and never call release() more than once
#12865
Conversation
… called and make sure release() is only called once Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
| @@ -28,6 +28,7 @@ public class BrotliDecoderSource extends DecoderSource | |||
| { | |||
| private final DecoderJNI.Wrapper decoder; | |||
| private final BrotliCompression compression; | |||
| private boolean released; | |||
There was a problem hiding this comment.
This is unnecessary, as release() is called from ContentSourceTransformer.finish() and it is already protected from multiple invocations there.
Likewise, isReleased() could be moved to ContentSourceTransformer.
There was a problem hiding this comment.
I added isReleased() only to test that the DecoderSource implementation received the release() call. It's not meant to be a useful API.
| @@ -43,6 +43,7 @@ private enum State | |||
| private int size; | |||
| private long value; | |||
| private byte flags; | |||
| private boolean released; | |||
| finished = true; | ||
| release(); | ||
| } | ||
| finish(); |
There was a problem hiding this comment.
Uhm, so finished is only used to early return EOF.
Here we handle rawChunk failures, so I think it was wrong before that finished was set; we should just call release() and make sure it is called only once in case of multiple reads.
Perhaps rename finished to eof, finish() to terminate() and use a boolean terminated to guarantee only one call to release().
| finished = true; | ||
| release(); | ||
| } | ||
| finish(); |
There was a problem hiding this comment.
Here also, if terminated==true, set eof=true, but if terminalFailure==true we only need to call terminate() (see previous comment about renaming).
| needsRawRead = false; | ||
| if (rawChunk != null) | ||
| rawChunk.release(); | ||
| rawChunk = Content.Chunk.from(failure, true); | ||
| if (transformedChunk != null) | ||
| transformedChunk.release(); | ||
| transformedChunk = rawChunk; | ||
| finish(); |
There was a problem hiding this comment.
This needs to be re-evaluated.
As failures are asynchronous, we want to do as little as possible here to avoid having to use a lock.
Possibly just delegating to the rawSource.
The reason is that there may be non-failed transformed content to read, and possibly the failure has been serialized in the rawSource so reading would be possible, and only eventually the failure is detected (for example a queued rawSource).
The idea was to do as little as possible, as the failure would be handled by the rawSource, and will be eventually seen via read().
There was a problem hiding this comment.
Remember that after a call to fail(), all resources should have been released, even if you may decide to stop reading entirely.
Signed-off-by: Ludovic Orban <[email protected]>
...-brotli/src/main/java/org/eclipse/jetty/compression/brotli/internal/BrotliDecoderSource.java
Outdated
Show resolved
Hide resolved
...ession-gzip/src/main/java/org/eclipse/jetty/compression/gzip/internal/GzipDecoderSource.java
Outdated
Show resolved
Hide resolved
...d/src/main/java/org/eclipse/jetty/compression/zstandard/internal/ZstandardDecoderSource.java
Outdated
Show resolved
Hide resolved
…current failures. Signed-off-by: Simone Bordet <[email protected]>
Fixes #12855