Clone PerMessageDeflateExtension values correctly

[marci4]

Description

Related Issue

Fixes #1437
Fixes #1400

Motivation and Context

Issue

How Has This Been Tested?

Additional tests

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[robert-s-ubi]

Please insert:

• clone.setDeflater(this.getDeflater());
• clone.setInflater(this.getInflater());

and replace the new Inflater() calls in lines 169 and 177 with inflater.reset(), and the deflater.end() & new Deflater() calls in lines 247/248 with deflater.reset().

[marci4]

We are not going to reuse the deflater/inflater for multiple clients

[marci4]

Using reset however should be done, I agree

[robert-s-ubi]

We are not going to reuse the deflater/inflater for multiple clients

Could you elaborate? If the user of this library wants to hand in a different inflater/deflater, why should that not be used?

Specific use case: I want to use BEST_COMPRESSION, not DEFAULT_COMPRESSION, to get the most out of compression (and both my servers and clients are powerful enough to put up that extra effort). That's why I hand in a new Deflater(Deflater.BEST_COMPRESSION, true) instance. I'd like that to work.

[marci4]

The problem is that we create a new instance of a Draft (and therefore the provided extension with the custom deflater/inflater) for every new client connecting to the server.
https://github.com/TooTallNate/Java-WebSocket/blob/master/src/main/java/org/java_websocket/WebSocketImpl.java#L272

If we simply use the single instance provided by the user, we will get in trouble for the server implementation

[robert-s-ubi]

If we simply use the single instance provided by the user, we will get in trouble for the server implementation

Ah, thanks, I see. I was just lucky that I was only using 1 server and client for my WebSocket compression tests.

Indeed, then an API change is needed to set Inflater and Deflater factories. Or, if we think that this flexibility is not needed, a setCompressionLevel(int level); API would do for me. Then this class would always instantiate the Inflater and Deflater objects, always with nowrap=true.

[marci4]

Looking more into the documentation, I think that nowrap will always be true in our case

[marci4]

If we simply use the single instance provided by the user, we will get in trouble for the server implementation

Ah, thanks, I see. I was just lucky that I was only using 1 server and client for my WebSocket compression tests.

Indeed, then an API change is needed to set Inflater and Deflater factories. Or, if we think that this flexibility is not needed, a setCompressionLevel(int level); API would do for me. Then this class would always instantiate the Inflater and Deflater objects, always with nowrap=true.

Yeah I think so too. We only need the compression level, nowrap should be true.

@PhilipRoman you got any objections?

[marci4]

My bad, there is no getter for the level/strategy of a Deflater.

Therefore:
new API -> setCompressionLevel()
remove getter/setter for Deflater/Inflater

[PhilipRoman]

Looks good, although personally I would add some calls to setDeflaterLevel in the functional tests like testDecodeFrame.

[robert-s-ubi]

Unfortunately, changes are needed for Java 8 compatibility.

[robert-s-ubi]

Even though the Deflater Javadoc says the same, the implementation actually also accepts DEFAULT_COMPRESSION, which is -1. The getter above would then return -1 as well.

[robert-s-ubi]

Unfortunately, this does not work in Java 8:

While this matches the Java 8 Deflater Javadoc, the implementation does not match the documentation:
https://github.com/openjdk/jdk8u/blob/master/jdk/src/share/classes/java/util/zip/Deflater.java

The set level is never picked up as far as I can tell. The "setParams" boolean is never evaluated. It is in Java 11, though, so it works there:
https://github.com/openjdk/jdk11u/blob/master/src/java.base/share/classes/java/util/zip/Deflater.java

So, for Java 8 compatibility, the Deflater instance has to be created with the configured compression level.

How about not having setDeflaterLevel at all, but rather making it an optional constructor argument:

private final int compressionLevel;
private final Inflater inflater;
private final Deflater deflater;

public PerMessageDeflateExtension() {
this(Deflater.DEFAULT_COMPRESSION);
}

public PerMessageDeflateExtension(int compressionLevel) {
this.compressionLevel = compressionLevel;
this.inflater = new Inflater(true);
this.deflater = new Deflater(compressionLevel, true);
}

public int getCompressionLevel() {
return compressionLevel;
}

@OverRide
public IExtension copyInstance() {
PerMessageDeflateExtension clone = new PerMessageDeflateExtension(this.getCompressionLevel());
clone.setThreshold(this.getThreshold());
clone.setClientNoContextTakeover(this.isClientNoContextTakeover());
clone.setServerNoContextTakeover(this.isServerNoContextTakeover());
return clone;
}

[robert-s-ubi]

Looks good to me. Thanks for your patience!