-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Add new RFC 7692-compliant Per-Message Deflate extension implementation #1498
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
base: master
Are you sure you want to change the base?
Add new RFC 7692-compliant Per-Message Deflate extension implementation #1498
Conversation
Add a reimplementation of the WebSocket Per-Message Deflate extension written from scratch for full RFC 7962 compliance, with the following over the existing implementation: - compliant extension parameters negotiation handling - default operation with context takeover works - fragmented messages are handled correctly (either all fragments are compressed/decompressed or none) - clears RSV1 after decompressing to remove the compression mark - produces the result specified in RFC 7962 section 7.2.3.4 - produces the result specified in RFC 7962 section 7.2.3.6 - uses the extension common name registered for RFC 7962 - has an additional optional constructor parameter "maxFrameSize" to ensure the limit is not exceeded while decompressing already - has an additional API "getCompressionRatio()" to get the effective compression ratio (over all payloads compressed and decompressed) Otherwise, it is fully API compatible with the old implementation. For now, the new implementation lives side by side with the old one, and is named "WebSocketPerMessageDeflateExtension". Add RFC 7962 tests for the new implementation, to validate it produces the expected results for all examples from RFC 7962 section 7.2.3. Add a copy of the unit tests for the old implementation, which verifies the new implementation works the same, except for fixed issues and different defaults.
|
If desired, I could also make the new implementation replace the old one. |
|
@marci4 could you take a look at this? |
|
@robert-s-ubi sorry for the delay. I finally got around to check it. For the structure. For the implementation:
The following test cases for the server reported not implemented:
Do you think you can take a look at these? |
|
@marci4 thanks for taking the time! As to the test cases, 13.3 and 13.5 are indeed not supported, because the Java Deflater/Inflater classes do not support setting the maxWindowBits parameter (the underlying native zlib does, but that is not exposed through that class) and thus always operate with the default maxWindowBits 15, which is also the default for RFC 7692. And the getProvidedExtensionsAs*() methods omit this parameter when it is the default value - which it always is, thus the implementation will never include that extension parameter. So from the outside it looks like this extension parameter is not implemented at all, but internally, it is fully implemented, but limited to the capabilities of Java Deflater/Inflater. This limitation could be overcome by using a different Java library for zlib compression which offers this parameter, but I chose to stick to the standard Java classes for now. I'll add a commit which renames the new implementation and test so that it replaces the old ne. |
Instead of adding WebSocketPerMessageDeflateExtension side by side with the old PerMessageDeflateExtension, rename the new implementation to the old name to replace it.
|
@marci4 I have added a commit that makes the new implementation replace the old one. The tests failed in Issue997Test which seems to have a stability issue unrelated to my changes. I cannot retrigger the CI run, maybe you can? |
|
@robert-s-ubi dont worry about this test ;) |
I found that I can retrigger the checks by closing and reopening the PR, and finally got all checks to pass. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a new RFC 7692-compliant WebSocket Per-Message Deflate extension implementation to address multiple compliance and correctness issues in the existing implementation. The new implementation is named PerMessageDeflateExtension and lives alongside the old implementation.
Key changes:
- Complete rewrite of the Per-Message Deflate extension with proper RFC 7692 compliance
- New features including
maxFrameSizeparameter andgetCompressionRatio()API - Changed default behavior for context takeover (now enabled by default) and compression threshold (64 bytes instead of 1024)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 27 comments.
| File | Description |
|---|---|
src/main/java/org/java_websocket/extensions/permessage_deflate/PerMessageDeflateExtension.java |
Complete reimplementation with proper RFC 7692 compliance, improved parameter negotiation, and new APIs |
src/test/java/org/java_websocket/extensions/PerMessageDeflateExtensionRFC7962Test.java |
New RFC 7692 compliance tests validating all examples from RFC 7692 section 7.2.3 |
src/test/java/org/java_websocket/extensions/PerMessageDeflateExtensionTest.java |
Updated existing tests to reflect changed defaults and fixed behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/test/java/org/java_websocket/extensions/PerMessageDeflateExtensionRFC7962Test.java
Outdated
Show resolved
Hide resolved
src/test/java/org/java_websocket/extensions/PerMessageDeflateExtensionRFC7962Test.java
Outdated
Show resolved
Hide resolved
src/main/java/org/java_websocket/extensions/permessage_deflate/PerMessageDeflateExtension.java
Outdated
Show resolved
Hide resolved
src/test/java/org/java_websocket/extensions/PerMessageDeflateExtensionRFC7962Test.java
Outdated
Show resolved
Hide resolved
src/test/java/org/java_websocket/extensions/PerMessageDeflateExtensionRFC7962Test.java
Outdated
Show resolved
Hide resolved
src/test/java/org/java_websocket/extensions/PerMessageDeflateExtensionRFC7962Test.java
Outdated
Show resolved
Hide resolved
src/test/java/org/java_websocket/extensions/PerMessageDeflateExtensionRFC7962Test.java
Outdated
Show resolved
Hide resolved
src/test/java/org/java_websocket/extensions/PerMessageDeflateExtensionRFC7962Test.java
Outdated
Show resolved
Hide resolved
src/test/java/org/java_websocket/extensions/PerMessageDeflateExtensionRFC7962Test.java
Outdated
Show resolved
Hide resolved
src/test/java/org/java_websocket/extensions/PerMessageDeflateExtensionRFC7962Test.java
Outdated
Show resolved
Hide resolved
RFC 7692 was often mistyped as RFC 7962. Fix this.
The PerMessageDeflationExtension's optional maxFrameSize constructor parameter is applied to the size of a fragment while decompressing, and not to the complete frame size (that size is checked after passing the decompressed fragment), so rename the parameter to what it actually is. Add Javadoc to the full-parameter constructor method.
This new API was intended to be called by library users, but could never have worked, as it would need to be called on the class instance which is actually processing frames, but that instance is created internally in the library using copyInstance() and thus inaccessible to the library users. Remove the API and the stats kept for it.
This reverts commit c465a9a.
The PerMessageDeflateExtension#getCompressionRatio() API needs to be called on the class instance used by the library. Add Javadoc explaining how that instance can be retrieved.
|
Following the AI review, I:
And now it passed the checks, too. |
Description
Add a reimplementation of the WebSocket Per-Message Deflate extension written from scratch for full RFC 7692 compliance, with the following over the existing implementation:
Otherwise, it is fully API compatible with the old implementation.
Add RFC 7692 tests for the new implementation, to validate it produces the expected results for all examples from RFC 7692 section 7.2.3.
Adapt the existing unit tests to the corrected behavior and changed defaults.
Related Issue
This fixes issue #1496
Motivation and Context
The existing implementation was broken in multiple ways and not fully RFC 7692 compliant. The new implementation was written to fix all that.
How Has This Been Tested?
Tested with adapted existing tests for the existing implementation, new RFC 7692 compliance tests (all part of this PR), and also within the Java-OCA-OCPP library, and an integration test which creates an OCPP client and server and connects these on the local machine.
Types of changes
Checklist: