TooTallNate / Java-WebSocket

A barebones WebSocket client and server implementation written in 100% Java.
http://tootallnate.github.io/Java-WebSocket
MIT License
10.54k stars 2.58k forks source link

Question: How can the compression threshold be set for the PerMessageDeflateExtension in a Deflate Client? #1437

Closed CommuniG8 closed 2 weeks ago

CommuniG8 commented 2 months ago

Describe what you would like to know or do I'd like to be able to set the compression threshold to a lower value than the 1024 byte value in the PerMessageDeflateExtension.

Describe the solution you'd considered I've taken a copy of the 1.5.7 source code and changed the default threshold which does work but this is not ideal.

Additional context I'm sending messages which are a little smaller than 1024 bytes that I'd like to compress.

PhilipRoman commented 2 months ago

setThreshold should do what you want

CommuniG8 commented 2 months ago

Could you give an example of using that method please?

CommuniG8 commented 2 months ago

It's not obvious how it should be called.

marci4 commented 1 month ago

Question is answered

PhilipRoman commented 1 month ago

Actually I am not sure it is solved - from a brief look I didn't see a way for user to access the internal objects. But at the moment I don't have a lot of time and this is a low priority issue. It's possible we need to add a setter for this.

marci4 commented 1 month ago

What internal classes do you mean?

marci4 commented 1 month ago

Oh I see the problem. The values are not correctly cloned. https://github.com/TooTallNate/Java-WebSocket/blob/master/src/main/java/org/java_websocket/extensions/permessage_deflate/PerMessageDeflateExtension.java#L333

Looking into copyInstance(), the fix for us for the specific issue is easy. The problem however is that setInflater/setDeflater.

@PhilipRoman I think about removing setInflater/setDeflater and replace them with a specific factory which the user can provide if they want to. The setter never worked and therefore I dont think we break many existing applications. What do you think?

robert-s-ubi commented 1 month ago

Looking into copyInstance(), the fix for us for the specific issue is easy. The problem however is that setInflater/setDeflater.

@PhilipRoman I think about removing setInflater/setDeflater and replace them with a specific factory which the user can provide if they want to. The setter never worked and therefore I dont think we break many existing applications. What do you think?

I would prefer to fix setInflater/setDeflater. It is in fact not hard to do: Replace the inflater.end() + new Inflater() lines with inflater.reset(), and do the same for the deflater. Then the set inflater/deflater objects remain preserved. I tried that and it worked correctly for me.

PhilipRoman commented 1 month ago

@CommuniG8 here is example how to use setTimeout:

PerMessageDeflateExtension ext = new PerMessageDeflateExtension();
ext.setThreshold(...);
Draft perMessageDeflateDraft = new Draft_6455(ext);
// then just pass the Draft object to websocket server/client constructor
new WebSocketClient(..., perMessageDeflateDraft)
new WebSocketServer(..., Collections.singletonList(perMessageDeflateDraft))

Once the bug is fixed this should work

@marci4 I don't have a strong opinion either way

marci4 commented 1 month ago

Looking into copyInstance(), the fix for us for the specific issue is easy. The problem however is that setInflater/setDeflater. @PhilipRoman I think about removing setInflater/setDeflater and replace them with a specific factory which the user can provide if they want to. The setter never worked and therefore I dont think we break many existing applications. What do you think?

I would prefer to fix setInflater/setDeflater. It is in fact not hard to do: Replace the inflater.end() + new Inflater() lines with inflater.reset(), and do the same for the deflater. Then the set inflater/deflater objects remain preserved. I tried that and it worked correctly for me.

I am not doing a lot of java in my daily work, so please forgive me. But I do not see a way to access the "nowrap" for the following constructor calls:

public Deflater(int level, boolean nowrap)
public Inflater(boolean nowrap)
robert-s-ubi commented 1 month ago

I am not doing a lot of java in my daily work, so please forgive me. But I do not see a way to access the "nowrap" for the following constructor calls:

public Deflater(int level, boolean nowrap)
public Inflater(boolean nowrap)

If the user hands in a deflater/inflater instance, the user takes control of the compression. It might not be PKZIP or GZIP at all. So I think it makes no difference that the user has control over this parameter. Or in other words: It is the user's responsibility to hand in an instance with compatible compression.

marci4 commented 1 month ago

Let us move the discussion to the PR.