GzipHandler: NPE in setDeflaterPoolCapacity and setInflaterPoolCapacity

[mperktold]

Jetty version
10.0.1

Description
There is a NPE when calling GzipHandler.setDeflaterPoolCapacity or GzipHandler.setInflaterPoolCapacity. The given capacity is passed to the setter of the inflater/deflater pool, but the pool is null before the handler is started, and it is illegal to call the methods on a started handler.

The bug was introduced as a combination of 3bdd82e and 0e3cfe8.

Another minor thing: The JavaDoc of setInflaterPoolCapacity mentions "DeflaterPool" instead of "InflaterPool".

[sbordet]

@mperktold would you like to submit a pull request?

[mperktold]

@sbordet I can do that.

Would you prefer to introduce separate fields for the capacities, or to create the pools in the constructor?

[sbordet]

@mperktold to create the pools you need the server, which is not passed to the constructor, so go with separate fields for the capacities.

[gregw]

Note we will be doing a 10.0.2 release soon, so if @mperktold can do a PR soon then great... if not @lachlan-roberts can you look at this before 10.0.2

[mperktold]

I wanted to pass the new capacity fields to the corresponding setter of the pools after creating them in doStart, but that doesn't work since the pools returned from ensurePool methods are already started, and changing capacity afterwards is illegal.

Should I pass the capacity as an additional parameter to ensurePool, leaving the current signature as an overload that passes CompressionPool.DEFAULT_CAPACITY?
Should a custom capacity take precedence over CompressionPool.DEFAULT_CAPACITY only or also over the size of the thread pool?

Does it even make sense to specify a custom capacity?

[lachlan-roberts]

I think we should actually just deprecate both these methods. It doesn't seem much correct to have setDeflaterPoolCapacity() and setInflaterPoolCapacity() on the GzipHandler when it is using CompressionPools shared with the whole server.

If you wish to use a pool with a custom capacity you should create the pool yourself and add it as a bean to the server. Then the call to ensure pool in the GzipHandler will return the pre-configured pool it finds on the server.

[mperktold]

@lachlan-roberts That sounds reasonable to me.

If we choose this path, the deprecated capacity setters wouldn't do anything apart from throwing if already started, right?

[gregw]

I think the deprecated methods should log warnings if called, but otherwise be a noop, but ok to throw if started.

[gregw]

Also planning to build releases in the next 24 to 48 hours

[lachlan-roberts]

@mperktold PR #6092 fixes this. It also adds setInflaterPool() and setDeflaterPool() methods to set a custom pool that GzipHandler will use. Otherwise it will use the servers pools which are shared with other things like websocket.

[mperktold]

Great! Yes, I saw you started working on it, thanks!👍