-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fix Per-Message Compression Extension support #1497
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?
Fix Per-Message Compression Extension support #1497
Conversation
The per-message compression extension was badly broken, leading to exceptions and loss of communication between client and server. Fix the extension negotiation to correctly send the current state of the client_no_context_takeover and server_no_context_takeover configuration variables, and ORing the received extensions with the received ones, to ensure client and server end up with the same configuration. Fix the application of these configuration variables, which must be swapped on the client vs. the server: The configuration of the deflater on the client side must affect the inflater on the server side, and vice versa. Remove the requestedParameters map as there is no need for it. This fixes issue TooTallNate#1496
|
@marci4 since you last fixed this class, would you maybe have the time to review and approve this PR? |
Resetting the inflater/deflater is never a "must". Only _not_ resetting the inflater is a "must" if the deflater is not being reset.
|
The failing CI run seems to be a server issue. I cannot find a button to rerun it. |
I try to take a look at it the next week. Checking it against the test suite as well |
|
@marci4 In the meantime, I have written a new RFC 7962 implementation from scratch, with tests validating all RFC 7962 examples, which I contributed to another project where I need it working: ChargeTimeEU/Java-OCA-OCPP#414 If interested, I could contribute the new implementation to this project as well. |
|
@robert-s-ubi feel free to contribute this as well |
Description
The per-message compression extension was badly broken, leading to exceptions and loss of communication between client and server.
Fix the extension negotiation to correctly send the current state of the client_no_context_takeover and server_no_context_takeover configuration variables, and ORing the received extensions with the received ones, to ensure client and server end up with the same configuration.
Fix the application of these configuration variables, which must be swapped on the client vs. the server: The configuration of the deflater on the client side must affect the inflater on the server side, and vice versa.
Remove the requestedParameters map as there is no need for it.
Related Issue
This fixes issue #1496
Motivation and Context
The broken extension negotiation and application of configuration made it impossible to use the compression extension WITH context takeover, which is the most efficient compression, and the only one which makes sense for applications using smaller message sizes, like OCPP.
With this fix, the Java-OCA-OCPP library can use efficient compression for OCPP messages.
How Has This Been Tested?
Tested within the Java-OCA-OCPP library, and an integration test which creates an OCPP client and server and connects these on the local machine. When enabling WebSocket compression with the existing library, the test always crashed with an Exception at the second message exchange, due to the inflater and deflater resets being incorrectly applied.
With this fix, the tests which crashed before all pass.
Types of changes
Checklist: