Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
CommuniG8 opened this issue Sep 3, 2024 · 12 comments · Fixed by #1439

Comments

@CommuniG8
Copy link

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
Copy link
Collaborator

setThreshold should do what you want

@CommuniG8 CommuniG8 changed the title Question: How can the compression threshold be set for the PerMessageDeflateExtension in a Delate Client? Question: How can the compression threshold be set for the PerMessageDeflateExtension in a Deflate Client? Sep 5, 2024
@CommuniG8
Copy link
Author

Could you give an example of using that method please?

@CommuniG8
Copy link
Author

It's not obvious how it should be called.

@marci4 marci4 added the Question label Oct 1, 2024
@marci4
Copy link
Collaborator

marci4 commented Oct 1, 2024

Question is answered

@marci4 marci4 closed this as completed Oct 1, 2024
@PhilipRoman
Copy link
Collaborator

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 marci4 reopened this Oct 1, 2024
@marci4
Copy link
Collaborator

marci4 commented Oct 1, 2024

What internal classes do you mean?

@marci4
Copy link
Collaborator

marci4 commented Oct 8, 2024

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
Copy link
Contributor

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
Copy link
Collaborator

PhilipRoman commented Oct 8, 2024

@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
Copy link
Collaborator

marci4 commented Oct 8, 2024

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
Copy link
Contributor

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
Copy link
Collaborator

marci4 commented Oct 8, 2024

Let us move the discussion to the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants