Implement Deflater / Inflater Object Pool

[joakime]

Bug #295 has exposed a JVM bug related to the permessage-deflate websocket extension.

To resolve it, a Deflater / Inflater Object Pool should be created and used by WebSocket.
With potential use by GzipHandler as well.

[mperktold]

+1 for using the proposed Deflator pool in GzipHandler, too.

In our production app using Jetty 9.4.9.v20180320 (which includes #1429), we experienced the following error:

java.lang.OutOfMemoryError: unable to create new native thread
    at java.lang.Thread.start0(Native Method)
    ...

That is, Java fails to allocate enough native memory, and we think that this is caused by GzipHandler
and its use of Deflaters.

According to our analysis, the problem is not that calling Deflater.end() does not help, but instead that it is never called when the thread that created the Deflater terminates due to inactivity, because in this way the thread-local Deflater becomes unreachable.

In principle, the Deflator could then be garbage-collected, so that its memory would be freed in finalize(), which calls end() internally. But, as described in the JVM bug 4797189 referenced in #293, the deflater allocates mostly non-Java memory, so the GC thinks that the deflater occupies only little memory and thus is not eager to free it.

Therefore, we worked around this issue by subclassing GzipHandler and calling deflater.end() inside recycle():

public class NonRecyclingGzipHandler extends GzipHandler {
    @Override
    public void recycle(Deflater deflater) { deflater.end(); }
}

This seems to prevent memory leaks, but of course is not ideal. A proper pool of Deflaters as proposed here, where instances can be shared by multiple threads, would certainly be a better solution.

[sbordet]

@mperktold yet, object pooling has its own problems.

For your case, how would you size the object pool? It's easy to have an infinite one, but that's not good.
A bounded pool is fairly easy too, but how do you know how to size the capacity?

Let's imagine you have a peak of requests and you fill the object pool to its capacity, but then you go idle. Is it ok to keep around all those Deflaters forever?

[sbordet]

@gregw @joakime what's your thought on this?

We can create a Deflater-specific pool, so that:

• if the pool is empty, we create a Deflater
• when a Deflater is returned to the pool, we can either reset() it (if it fits the pool) or end() it (if it overflows the pool)

Or, we can create a generic ObjectPool with a queue and a scheduler to time out objects, and then functions to be passed to the pool for those actions above:

• create(Supplier<T> supplier)
• recycle(Consumer<T> recycler)
• discard(Consumer<T> cleaner)

No matter which choice, recycle() is tricky since we must avoid to add it to the pool (where could be taker by others) without having run the recycler function - a coarser lock is needed.

[sbordet]

See also #2579.

[mperktold]

Those are some good questions. Here are some opinions on them, but keep in mind that, unlike you guys, I'm not an expert in this field. 😉

The pool should be sized and configured by passing parameters to the constructor, and then the configured pool is given to GzipHandler, like:

new GzipHandler(new DeflaterPool(lowerBound, upperBound, timeout, TimeUnit.SECONDS, scheduler));

A generic pool would be fine as well. The point is: the programmer should be able to configure the pool. It would also be nice to have some sensible default values for the configuration, just as in QueuedThreadPool.

The pool should clean up Deflater instances after a certain timeout. The programmer should be able to specify the timeout as well as the scheduler that checks the timeout.

Regarding the pool's capacity: I would say that the number of Deflater instances in the pool should be limited in order not to occupy too much memory. But when that limit is reached, the pool should not fail to return a Deflater, but rather create a new one. When a Deflater is returned to the pool and the pool has already reached its maximum, it should free and discard the Deflater instead of pooling it.
Edit:
Actually, this is exactly what you are proposing in your second comment.

Now if I were to choose a value for the upper bound, I would try to choose it high enough such that it is unlikely to be reached under normal load, but also not too high such that when it is reached under high load, there is still sufficient memory. Probably, some experiments are needed to find a good upper bound.

One possibility would be to set it equal to the upper bound of the ThreadPool of the Server, since that one already limits the number of concurrent requests accepted by the server.

[sbordet]

@joakime brought a good point that Deflater objects are configurable, so they are not all made equal, which means that you cannot pool them without taking into account their configuration, or reconfiguring them when borrowed from the pool.

[lachlan-roberts]

@gregw summary of hangout with @sbordet

We shouldn't be using ThreadLocal because if a thread times out from the ThreadPool deflater.end() will never be called.

Is a DeflaterPool actually needed when a new Deflater could be created and ended each time with multiple threads in parallel avoiding all potential lock contention on the pool?

Currently GzipFactory.recycle() is only being called when there are no failures. GzipHttpOutputInterceptor.GzipBufferCB needs to recycle or end the Deflater in onCompleteFailure() to make sure the Deflater is recycled in the case of a failure.

[sbordet]

@lachlan-roberts see above my comment on recycling Deflater instances.

The problem is that the moment thread1 offer() the Deflater to the pool, thread2 is entitled to poll() it out and use it, and now thread1 is calling reset() concurrently with thread2 using the Deflater.

[gregw]

@sbordet @lachlan-roberts When the "pool" was implemented, it was certainly a big performance win over creating new Deflaters each time... but that was a long time ago, so the JVM may well have changed significantly.

@lachlan-roberts Can you do a Simple JMH test that compares (creating a new Deflator, configuring it and compressing a single character) vs (taking a Deflator from a simple ConcurrentLinkedList pool and compressing a single character).

The recycle then reset problem is not a problem for a ThreadLocal based "pool", and should be trivial to fix for a real pool.

[mperktold]

This looks great, thanks! 😃

[bk-mz]

Reopened as we still need Inflater pool and to use pools in websocket extensions

Actually, I think with current design it's not possible to implement Inflater/Deflater pool for websocket extensions.

The thing is that currently websockets try to comply with rfc7692, which states that, roughly, compressor-decompressor must accumulate message statistics to make compression more efficient.

More on that here https://www.igvita.com/2013/11/27/configuring-and-optimizing-websocket-compression/

Currently websocket extension maintain one Inflater/Deflater pair per one websocket connection. They both are (should be?) finalized on the end of websocket connection, as stated here:

https://github.com/eclipse/jetty.project/blob/jetty-9.4.x/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/extensions/compress/CompressExtension.java#L375

Usage of Deflater/Inflater pools will break current design and break current compatibility with RFC7692.

Something clearly should be done with websockets, as usage of compression still produces memory leaks. But this ticket should be closed, IMO)

[joakime]

The problem lies in the JVM.
Calling Deflater.end() or Inflater.end() does not address the memory leaks we encounter.
The memory does not get cleared up if the Deflater or Inflater still has content.
Clearing out the content doesn't work.
.reset() doesn't work entirely as well - the memory still held/referenced until you use the compressor again (first input will overwrite the old referenced memory that would cause the leak, rendering that instance now safe to use again)

Using a Inflater/Deflater pool would require buckets that correspond to the same "configuration" of a Deflater/Inflater (compression level, wrap mode, flush mode).
But that's about it, the pool would .reset() each pooled instance before returning it, side-stepping the memory leak, and satisfying the various requirements of the possible configurations outlined in the handshake of RFC7692.
The websocket extension would still CompressionPool.acquire() per websocket handshake and CompressionPool.release() with each completed websocket closure, making the compression state/context/window sane for the duration of the websocket connection.

[bk-mz]

So you're saying, the leak could be fixed just by using Inflater/Deflater pool in CompressionExtension? Well, we could give it at least a try, since naive implementation is not very hard...

[bk-mz]

@joakime did I get you right? https://github.com/eclipse/jetty.project/pull/3518/files

[bk-mz]

Just a couple of references to current talk from zlib manual:

ZEXTERN int ZEXPORT deflateEnd OF((z_streamp strm));

All dynamically allocated data structures for this stream are freed. This function discards any unprocessed input and does not flush any pending output.
deflateEnd returns Z_OK if success, Z_STREAM_ERROR if the stream state was inconsistent, Z_DATA_ERROR if the stream was freed prematurely (some input or output was discarded). In the error case, msg may be set but then points to a static string (which must not be deallocated).

ZEXTERN int ZEXPORT deflateReset OF((z_streamp strm));

This function is equivalent to deflateEnd followed by deflateInit, but does not free and reallocate the internal compression state. The stream will leave the compression level and any other attributes that may have been set unchanged.

ZEXTERN int ZEXPORT deflateInit OF((z_streamp strm, int level));
Initializes the internal stream state for compression. The fields zalloc, zfree and opaque must be initialized before by the caller. If zalloc and zfree are set to Z_NULL, deflateInit updates them to use default allocation functions.
The compression level must be Z_DEFAULT_COMPRESSION, or between 0 and 9: 1 gives best speed, 9 gives best compression, 0 gives no compression at all (the input data is simply copied a block at a time). Z_DEFAULT_COMPRESSION requests a default compromise between speed and compression (currently equivalent to level 6).

deflateInit returns Z_OK if success, Z_MEM_ERROR if there was not enough memory, Z_STREAM_ERROR if level is not a valid compression level, Z_VERSION_ERROR if the zlib library version (zlib_version) is incompatible with the version assumed by the caller (ZLIB_VERSION). msg is set to null if there is no error message. deflateInit does not perform any compression: this will be done by deflate().

[bk-mz]

Ok, some time passed, we got some results on testing that.

First: I guess it's inevitable, but if you use java.util.zip you have to move to java11, as they fixed finalizer issues with those objects there (https://bugs.openjdk.java.net/browse/JDK-8187485)

The memory still does not return to OS, but this is due nature of G1 garbage collection -- a rather hearsay, but currently we got nothing better to explain this. That said, full explicit GC helps.

Meaning, if you have java11, explicit GC will gather all the memory even if you use current implementation of PerMessageDeflate.

[bk-mz]

The attempt to implement Deflater/Inflater pools for PerMessageDeflate is here: #3518

It also would address memory leaks, even for jdk8

[joakime]

Work has been completed in jetty-9.4.x branch and should be available in 9.4.20 release.