Issue #8769 - Introduce new Compression Handler with support for gzip, brotli, and zstandard #12075

joakime · 2024-07-22T23:27:56Z

Introduces new jetty-compression libs.

jetty-compression-common is the common location for Compression classes (neither server or client specific, just compression)
jetty-compression-gzip is the implementation of the generic compression lib to support Content-Encoding: gzip
jetty-compression-brotli is the implementation of the generic compression lib to support Content-Encoding: br (brotli)
jetty-compression-zstandard is the implementation of the generic compression lib to support Content-Encoding: zstd (zstandard)
jetty-compression-server is the implementation of the DynamicCompressionHandler (and associated Request/Response wrappers)
jetty-compression-tests is the integration tests of features.

Fixes: #8769

...10-demos/jetty-ee10-demo-embedded/src/main/java/org/eclipse/jetty/ee10/demos/OneHandler.java

gregw · 2024-08-20T07:04:03Z

@joakime what is the status of this? Should we be looking closer at it?

gregw

Just some initial thoughts... all reading well so far!

...compression-server/src/main/java/org/eclipse/jetty/compression/server/CompressionConfig.java

...ion-server/src/main/java/org/eclipse/jetty/compression/server/DynamicCompressionHandler.java

lorban

Here is my first batch of comments.

lorban · 2024-10-07T08:16:43Z

...mpression/jetty-compression-api/src/main/java/org/eclipse/jetty/compression/Compression.java

+    private final String etagSuffixQuote;
+    private ByteBufferPool byteBufferPool;
+    private Container container;
+    private int bufferSize = 2048;


Is there a specific reason to use 2K? That looks a bit small to me.

There are a few buffer size calculations present here.
There's the default size, typically used for non-compression related stuff, this is the 2kb you are seeing here.
There's Encoder specific buffer sizes.
There's Decoder specific buffer sizes.

See:

gzip - GzipDecoderConfig - 512 bytes as default - taken from java.util.zip.GZIPInputStream codebase

gzip - GzipEncoderConfig - 512 bytes as default - taken from java.util.zip.GZIPOutputStream codebase

zstd - ZstandardDecoderConfig - 64k typical - calculated from ZstdInputStreamNoFinalizer.recommendedDOutSize()

zstd - ZstandardEncoderConfig - 256k typical - calculated from ZstdOutputStreamNoFinalizer.recommendedCOutSize()

brotli - BrotliDecoderConfig - 16384 bytes - taken from Brotli4j codebase

brotli - BrotliEncoderConfig - 4096 bytes - taken from Brotli4j codebase.

As you can see, the value for "buffer size" is quite varied, depends on the implementation, and if you are encoding or decoding.

...mpression/jetty-compression-api/src/main/java/org/eclipse/jetty/compression/Compression.java

lorban · 2024-10-07T08:22:14Z

...mpression/jetty-compression-api/src/main/java/org/eclipse/jetty/compression/Compression.java

+     * @param length the requested size of the buffer
+     * @return the ByteBuffer suitable for this compression implementation.
+     */
+    public abstract RetainableByteBuffer acquireByteBuffer(int length);


I think internally wrapping a given ByteBufferPool.Sized with one that does all the magic about byte order, directness and size would be clearer instead of introducing these acquire methods.

I did that originally, but the sheer amount of code to do that was just off-putting.

Also, the zero length acquire rules with retainable were quite confusing, and the likely reason that we do a zero sized check in so many places we use ByteBufferPool.acquire still exists.

lorban · 2024-10-07T08:22:47Z

...mpression/jetty-compression-api/src/main/java/org/eclipse/jetty/compression/Compression.java

+    private final String encodingName;
+    private final String etagSuffix;
+    private final String etagSuffixQuote;
+    private ByteBufferPool byteBufferPool;


I think you want to use a ByteBufferPool.Sized (which is a new Jetty 12.1 concept)

I can only talk about the decision to use this vs ByteBufferPool.Sized at the moment.
I'll change it to ByteBufferPool.Sized now, and see what happens.
The history: The ByteBufferPool.Sized was quite buggy when I wrote this. It didn't behave properly at that point in time.
Also, at the time, (no longer true), the raw ByteBuffer was going through internals of the compression libs and using the RetainableByteBuffer concepts it made it difficult to track the specific buffer that was held in the compression lib to the buffer from this pool.

This also aligns with the default ArrayByteBufferPool from the server, which isn't a ByteBufferPool.Sized.

If a ByteBufferPool is unconfigured here, then the default ByteBufferPool.NON_POOLING is used, and since not all ByteBufferPool implementations are ByteBufferPool.Sized, this was the best and most flexible solution. If the implementation wants to use ByteBufferPool.Sized, then a call to .setByteBufferPool(new ByteBufferPool.Sized()) will still work here.

lorban · 2024-10-07T08:26:50Z

...mpression/jetty-compression-api/src/main/java/org/eclipse/jetty/compression/Compression.java

+     * @return the {@link InputStream} implementation for this compression.
+     * @throws IOException if unable to create InputStream
+     */
+    public abstract InputStream newDecoderInputStream(InputStream in, DecoderConfig config) throws IOException;


I don't think you should expose any method offering InputStream usage as Content.Source already offers a InputStream asInputStream(Content.Source source) static helper for this purpose.

This uses the compression implementation version of the InputStream, not the wrapped version of a wrapped of an extension. It's mostly used for tests and demonstration purposes. It will come to be especially handy for Jetty Client users in the future (when this is integrated there) as well.

...mpression/jetty-compression-api/src/main/java/org/eclipse/jetty/compression/EncoderSink.java

lorban · 2024-10-08T09:02:17Z

...compression-brotli/src/main/java/org/eclipse/jetty/compression/brotli/BrotliCompression.java

+
+        // Can Brotli4J use direct byte buffers?
+        RetainableByteBuffer buffer = getByteBufferPool().acquire(length, false);
+        buffer.getByteBuffer().order(getByteOrder());


If you change the pooled buffers' byte ordering, this should be reset once the buffer is released.

Either that's a job for yet another wrapping pool, or maybe build that into our existing pools.

Our existing pools do that by calling BufferUtil.reset() - tho the entire new set of pools should probably have beefed up testing to ensure that this is the case. (btw, this order change behavior is also present in GzipHandler's GzipResponseAndCallback)

jetty.project/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/handler/gzip/GzipResponseAndCallback.java

Lines 343 to 351 in a46dc6f

if (_buffer == null)

{

_buffer = getRequest().getComponents().getByteBufferPool().acquire(_bufferSize, false);

ByteBuffer byteBuffer = _buffer.getByteBuffer();

// Per RFC-1952, GZIP is LITTLE_ENDIAN

byteBuffer.order(ByteOrder.LITTLE_ENDIAN);

BufferUtil.flipToFill(byteBuffer);

// Add GZIP Header

byteBuffer.put(GZIP_HEADER, 0, GZIP_HEADER.length);

lorban · 2024-10-08T09:23:10Z

...mpression-brotli/src/main/java/org/eclipse/jetty/compression/brotli/BrotliDecoderSource.java

+    @Override
+    protected void release()
+    {
+        decoder.destroy();


It looks like making DecoderSource Closeable would be more representative as you're not releasing reusable resources but cleaning up.

Interesting, but then the users of this Decoder.Source have to be mindful to call close() on it, a new concept that Content.Source doesn't have. Also, only brotli has this requirement, gzip and zstd do not. (well, gzip has a variation of this requirement)

...jetty-compression/jetty-compression-server/src/main/config/etc/jetty-compression-handler.xml

lorban · 2024-10-08T10:24:05Z

...on-zstandard/src/main/java/org/eclipse/jetty/compression/zstandard/ZstandardCompression.java

+            throw new IllegalStateException("ByteBufferPool does not return zstd-jni required direct ByteBuffer");
+        }
+        // We rely on the ByteBufferPool.release(ByteBuffer) performing a ByteBuffer order reset to default (big-endian).
+        // Typically, this is done with a BufferUtil.reset(ByteBuffer) call on.


Adding a test to the current pool's suite to ensure byte ordering is properly reset sounds advisable.

Since GzipHandler already does this ByteBuffer.order change (after acquire), I don't see this as a big deal.

If we decide to address this with more testing, then we should do it for all of our new ByteBufferPool implementations and also review other places where this order change occurs.

That would make for a good (but separate) PR.

Opened a separate (initial) PR for this at #12432

gregw

I'm primarily reviewing for naming and structure at the moment.
Mostly good, but a few comments so far...

jetty-core/jetty-compression/jetty-compression-api/src/main/config/modules/compression-api.mod

...y-core/jetty-compression/jetty-compression-gzip/src/main/config/modules/compression-gzip.mod

...re/jetty-compression/jetty-compression-brotli/src/main/config/modules/compression-brotli.mod

...compression-server/src/main/java/org/eclipse/jetty/compression/server/CompressionConfig.java

...ompression-server/src/main/java/org/eclipse/jetty/compression/server/CompressionHandler.java

...mpression/jetty-compression-api/src/main/java/org/eclipse/jetty/compression/Compression.java

...compression-server/src/main/java/org/eclipse/jetty/compression/server/CompressionConfig.java

gregw

I think this is good enough to merge as an experimental handler.

with gzip, brotli, and zstandard. * New experimental CompressionHandler that is an eventual replacement for GzipHandler

joakime changed the title ~~Issue #8769 - Implement Dynamic Compression Handler with support for gzip, brotli, and zstd~~ Issue #8769 - Implement Dynamic Compression Handler with support for gzip, brotli, and zstandard Jul 22, 2024

joakime commented Jul 22, 2024

View reviewed changes

...10-demos/jetty-ee10-demo-embedded/src/main/java/org/eclipse/jetty/ee10/demos/OneHandler.java Outdated Show resolved Hide resolved

joakime linked an issue Jul 22, 2024 that may be closed by this pull request

Introduce CompressionHandler to support compression from gzip, brotli, and zstandard #8769

Closed

joakime mentioned this pull request Jul 22, 2024

Jetty Brotli compression support for on-the-fly compression? #2553

Closed

joakime self-assigned this Jul 22, 2024

joakime added the Enhancement label Jul 22, 2024

gregw requested review from gregw and lachlan-roberts August 6, 2024 23:35

gregw reviewed Aug 20, 2024

View reviewed changes

...compression-server/src/main/java/org/eclipse/jetty/compression/server/CompressionConfig.java Outdated Show resolved Hide resolved

...ion-server/src/main/java/org/eclipse/jetty/compression/server/DynamicCompressionHandler.java Outdated Show resolved Hide resolved

joakime mentioned this pull request Aug 21, 2024

Please release version 1.17.0 hyperxpro/Brotli4j#169

Closed

joakime changed the title ~~Issue #8769 - Implement Dynamic Compression Handler with support for gzip, brotli, and zstandard~~ Issue #8769 - Introduce new Compression Handler with support for gzip, brotli, and zstandard Aug 21, 2024

joakime mentioned this pull request Sep 11, 2024

Introduce BufferUtil.ensureDirect(ByteBuffer) method #12262

Closed

joakime mentioned this pull request Sep 27, 2024

Response compression does not work when the Accept-Encoding: * request header is used. #12324

Closed

joakime marked this pull request as ready for review October 2, 2024 21:10

joakime requested a review from gregw October 3, 2024 11:59

lorban reviewed Oct 8, 2024

View reviewed changes

gregw requested changes Oct 24, 2024

View reviewed changes

joakime mentioned this pull request Oct 28, 2024

Start of a testcases for ByteBufferPool contract #12432

Closed

gregw approved these changes Oct 30, 2024

View reviewed changes

Introduce Compression API for handling HTTP compression

c3565d5

with gzip, brotli, and zstandard. * New experimental CompressionHandler that is an eventual replacement for GzipHandler

joakime force-pushed the fix/12.1.x/dynamic-compression-handler branch from 045c094 to c3565d5 Compare October 30, 2024 12:27

joakime merged commit 8dbd278 into jetty-12.1.x Oct 30, 2024
5 of 8 checks passed

joakime deleted the fix/12.1.x/dynamic-compression-handler branch October 30, 2024 12:28

This was referenced Nov 27, 2024

Support zstandard encoded request/response content #6600

Closed

Introduce CompressionHandler to support compression from gzip, brotli, and zstandard #8769

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #8769 - Introduce new Compression Handler with support for gzip, brotli, and zstandard #12075

Issue #8769 - Introduce new Compression Handler with support for gzip, brotli, and zstandard #12075

joakime commented Jul 22, 2024 •

edited

Loading

gregw commented Aug 20, 2024

gregw left a comment

lorban left a comment

lorban Oct 7, 2024

joakime Oct 28, 2024

lorban Oct 7, 2024

joakime Oct 28, 2024

joakime Oct 28, 2024

lorban Oct 7, 2024

joakime Oct 28, 2024

joakime Oct 28, 2024

joakime Oct 28, 2024

lorban Oct 7, 2024

joakime Oct 28, 2024

lorban Oct 8, 2024

joakime Oct 28, 2024 •

edited

Loading

lorban Oct 8, 2024

joakime Oct 28, 2024

lorban Oct 8, 2024

joakime Oct 28, 2024

joakime Oct 28, 2024

gregw left a comment

gregw left a comment

	if (_buffer == null)
	{
	_buffer = getRequest().getComponents().getByteBufferPool().acquire(_bufferSize, false);
	ByteBuffer byteBuffer = _buffer.getByteBuffer();
	// Per RFC-1952, GZIP is LITTLE_ENDIAN
	byteBuffer.order(ByteOrder.LITTLE_ENDIAN);
	BufferUtil.flipToFill(byteBuffer);
	// Add GZIP Header
	byteBuffer.put(GZIP_HEADER, 0, GZIP_HEADER.length);

Issue #8769 - Introduce new Compression Handler with support for gzip, brotli, and zstandard #12075

Issue #8769 - Introduce new Compression Handler with support for gzip, brotli, and zstandard #12075

Conversation

joakime commented Jul 22, 2024 • edited Loading

gregw commented Aug 20, 2024

gregw left a comment

Choose a reason for hiding this comment

lorban 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

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

joakime Oct 28, 2024 • edited Loading

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 left a comment

Choose a reason for hiding this comment

gregw left a comment

Choose a reason for hiding this comment

joakime commented Jul 22, 2024 •

edited

Loading

joakime Oct 28, 2024 •

edited

Loading