Add PerMessageDeflate Extension support, see #574 #866

haruntuncay · 2019-03-09T14:58:20Z

Description

Implemented support for PerMessageDeflateExtension, requested in #574.
Added an example file to show how to include the extension.

Related Issue

Fixes #574

Motivation and Context

New feature

How Has This Been Tested?

Added a new test file.

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.

haruntuncay · 2019-03-16T07:34:05Z

Hey @marci4, Codacy/PR Quality Review is complaining about the unused static variables. I will send a commit to fix it, but do you think should I comment them out until they are needed, or remove them completely ?

marci4 · 2019-03-17T02:44:17Z

Hey @haruntuncay,
You can leave it in there. Commenting it out would simple result in another error.

Sorry that this is not yet merged but as you know I am on vacation for 3 more weeks :)

Best regards,
Marcel

marci4 · 2019-03-17T02:44:54Z

Hey @haruntuncay,
You can leave it in there. Commenting it out would simple result in another error.

Sorry that this is not yet merged but as you know I am on vacation for 3 more weeks :)

Best regards,
Marcel

haruntuncay · 2019-03-17T06:52:33Z

Hi @marci4,
Okay, I will leave them in place.

Sorry that this is not yet merged but as you know I am on vacation for 3 more weeks :)

Absolutely no problem, have a nice and fun vacation :)

rauchg · 2019-03-19T03:13:30Z

@haruntuncay just chatted with @TooTallNate about granting you access to manage this repo

TooTallNate · 2019-03-19T03:23:07Z

Hey @haruntuncay, just sent you an invite to be a collaborator on the project. Thank you in advance for any work your put in. 🍻 on me if you ever give SF a visit!

haruntuncay · 2019-03-19T17:54:11Z

Hello @TooTallNate,
Thank you for this project and the invitation. I would love to help maintain and hopefully can do a good job.

May I ask, just out of curiosity, how did you decided to intive me to collaborate on this project ? (I ask this because I originally was chatting with the socket.io java client maintainers and was surprised to see this invitation. 😄 )

marci4 · 2019-04-07T17:19:27Z

Hello @haruntuncay,

sorry for the delayed response.

I was testing your changes against the autobahn testsuite (see here) and it seems that there is an issue in the encoding process.

Could you take a look at this?

Thank you!

Best regards,
Marcel

marci4 · 2019-04-07T17:20:08Z

src/main/java/org/java_websocket/extensions/permessage_deflate/PerMessageDeflateExtension.java

+
+    @Override
+    public boolean acceptProvidedExtensionAsClient(String inputExtension) {
+        String[] requestedExtensions = inputExtension.split(",");


Should we not split by ; ?

I believe specification states that extensions are seperated by commas( , ), and parameters to extensions are seperated by semi-colons( ; ).

Sec-WebSocket-Extensions = extension-list extension-list = 1#extension extension = extension-token *( ";" extension-param ) extension-token = registered-token registered-token = token extension-param = token [ "=" (token | quoted-string) ] ;When using the quoted-string syntax variant, the value ;after quoted-string unescaping MUST conform to the ;'token' ABNF.

An example from the spec:
Spec-WebSocket-Extensions: mux; max-channels=4; flow-control, deflate-stream
This example shows the usage of mux extension with parameters max-channels=4, flow-control extension with no params and deflate-stream extension with no parameters.

Chrome sends the following header:
permessage-deflate; client_max_window_bits
So we would have to split by , first and then by ; right?

Yes, that would be the case for parsing with parameters. I currently didn't take parameters into account since I used Java provided classes, but I can parse them if you like.

marci4 · 2019-04-07T17:20:19Z

src/main/java/org/java_websocket/extensions/permessage_deflate/PerMessageDeflateExtension.java

+
+    @Override
+    public boolean acceptProvidedExtensionAsServer(String inputExtension) {
+        String[] requestedExtensions = inputExtension.split(",");


haruntuncay · 2019-04-07T18:09:35Z

Hello @haruntuncay,

sorry for the delayed response.

I was testing your changes against the autobahn testsuite (see here) and it seems that there is an issue in the encoding process.

Could you take a look at this?

Thank you!

Best regards,
Marcel

Hello @marci4,
Hope you had a fun vacation and thank you for the review. 😄 The utf-8 issue is expected and the PMCE spec#compression part states that PMCE compressed data is not subject to UTF-8 checks.

Explanation from the PMCE spec#compression

The payload data portion in frames generated by a PMCE is not subject
to the constraints for the original data type. For example, the
concatenation of the output data corresponding to the application
data portion of frames of a compressed text message is not required
to be valid UTF-8. At the receiver, the payload data portion after
decompression is subject to the constraints for the original data
type again.

marci4 · 2019-04-07T18:40:22Z

Hello @marci4,
Hope you had a fun vacation and thank you for the review. smile The utf-8 issue is expected and the PMCE spec#compression part states that PMCE compressed data is not subject to UTF-8 checks.

Explanation from the PMCE spec#compression

The payload data portion in frames generated by a PMCE is not subject
to the constraints for the original data type. For example, the
concatenation of the output data corresponding to the application
data portion of frames of a compressed text message is not required
to be valid UTF-8. At the receiver, the payload data portion after
decompression is subject to the constraints for the original data
type again.

Hey,

had a amazing vacation, would have loved to stay there a few more months :)

I was thinking of a different issue but you can disable the check (when only RSV1 is set) in isValid of TextFrame.

There is however a real issue in the encoding process.
When I try to connect with chrome to an echo server and type "a", I get a "K" as response.

Best regards,
Marcel

haruntuncay · 2019-04-14T13:33:05Z

Hello @marci4,

Hope you are doing well.
I have been dealing with that issue you have pointed out and what autobahn has reported, and I have finally completed them. All autobahn compression tests pass successfully now.

I have also added a new class to conveniently parse extension requests and parameters.

Note: When testing with autobahn, I had to comment out the body of PerMessageDeflateExtension#encodeFrame because I believe autobahn expects what it has sent. Otherwise, compression is applied to the echo messages and causes fails.

marci4 · 2019-04-16T05:29:25Z

@haruntuncay I am really sorry but I was busy the last two days! Gonna take a look at your changes later!

marci4 · 2019-04-17T19:52:07Z

Something is still wrong after the first frame... Trying to pinpoint it down

haruntuncay · 2019-04-17T19:56:08Z

What kind of error are you facing ? Is it a problem with fragmented frames ? Also, did you comment out the body of encodeFrame method ?

marci4 · 2019-04-17T19:57:14Z

Was trying to connect with a chrome to the server, getting an invalid frame error.

Looks like the second server does not have RSV1 set...

haruntuncay · 2019-04-17T20:00:32Z

Could you please share the code you are using to connect with chrome ?
Also, you can checkout autobahn results from here.

marci4 · 2019-04-17T20:05:23Z

I simple use the "Simple WebSocket Client" extension and enter the server. Also added the PerMessageDeflateExtension to the draft of course.

ChatServer sources are here https://gist.github.com/marci4/78c96c1d1a52bac9cb00efbf987d62a6

Best regards,
Marcel

haruntuncay · 2019-04-17T20:51:11Z

@marci4 . Turns out it is a problem caused by the "Simple Websocket Client" extension.
This extension doesn't set RSV1 bit for frames like the spec requires to. A quick debug session revealed that this browser extension only sends "permessage-deflate" as a header and doens't actually support it.
After all, autobahn and jetty sockets was succesfully able to connect to it. You can see in the debugger that payload for the frame is plain text, too.

haruntuncay · 2019-04-20T11:51:00Z

Hello @marci4,
Is there anything you want me to check out or fix for this PR ?

marci4 · 2019-04-20T11:55:34Z

@haruntuncay I send you an email to move the steps to reproduce out of this PR.

haruntuncay · 2019-04-22T20:17:05Z

Hello @marci4,
Sorry for the late response. I just checked my email and I couldn't see the mail. Could you please re-send it ?
Thanks in advance.

haruntuncay · 2019-05-07T15:04:34Z

Hello @marci4,
I have updated the PR and just wanted notify. Thank you for all the time you've spent reviewing this PR.

marci4 · 2019-05-07T17:59:27Z

Moving this to release for 1.5.0 since Deflater.SYNC_FLUSH requires Java 1.7

NoahAndrews · 2019-05-07T18:14:21Z

If you use SYNC_FLUSH in 1.5, you should document that the library is only compatible with Android API 19 (4.4 KitKat) and up. This is not an issue for the app I'm using this in, but it may prevent other developers from updating this library.

… a class to conveniently parse an extension request

Canop · 2020-01-08T13:57:00Z

Hi,
I can't find any release date (nor activity) for the 1.5. Is there any forecast ?

marci4 · 2020-01-08T16:31:48Z

Hey @Canop,
No real plans yet.
Mainly since I am really busy with work and I would like to also do a 1.4.1 before this.

Apart from this you can use this feature without a new library version.
Copy the code into your project and include the extension in the constructor for Draft_6455().

Examples should be visible in the source changes here.

Best regards,
Marcel

Canop · 2020-01-08T16:42:09Z

Thanks for the answer. I'll probably do that.
Too bad it's not available in standard but I understand it's not always easy to maintain foss projects.

marci4 · 2020-03-16T18:52:03Z

Hey @haruntuncay,

how do you feel about merging this PR?

Best regards,
Marcel

haruntuncay · 2020-03-17T07:07:40Z

Hi @marci4,

If you're okay with it, it's also fine by me.

marci4 · 2020-03-17T07:18:05Z

thank you very very much for this contribution!

Best regards,
Marcel

marci4 added Client New Feature Server labels Apr 7, 2019

marci4 reviewed Apr 7, 2019

View reviewed changes

marci4 added the Breaking changes label May 7, 2019

marci4 added this to the Release 1.5.0 milestone May 7, 2019

haruntuncay added 8 commits November 16, 2019 23:57

Add PerMessageDeflate Extension support, see #574

db1c860

Fix: Clear RSV1 bit after decoding.

66f07a0

Ran Autobahn tests and made fixes based on autobahn reports and added…

391e184

… a class to conveniently parse an extension request

Fixed test case

71d5e00

Change integer rsv bytes to hex values for clarity

27c46ed

Add files via upload

032dde8

Delete index.html

dc3934e

Update PerMessageDeflateExtension to use Deflater.SYNC_FLUSH option.

48a35ce

NoahAndrews mentioned this pull request Mar 12, 2020

Release of 1.4.1 #989

Merged

8 tasks

marci4 merged commit 4232021 into TooTallNate:master Mar 17, 2020

marci4 mentioned this pull request Mar 17, 2020

Implementation of per message deflate extension #574

Closed

Add PerMessageDeflate Extension support, see #574 #866

Add PerMessageDeflate Extension support, see #574 #866

Conversation

haruntuncay commented Mar 9, 2019 • edited by marci4 Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

haruntuncay commented Mar 16, 2019

marci4 commented Mar 17, 2019

marci4 commented Mar 17, 2019

haruntuncay commented Mar 17, 2019 • edited Loading

rauchg commented Mar 19, 2019

TooTallNate commented Mar 19, 2019

haruntuncay commented Mar 19, 2019

marci4 commented Apr 7, 2019

marci4 Apr 7, 2019

Choose a reason for hiding this comment

haruntuncay Apr 7, 2019

Choose a reason for hiding this comment

marci4 Apr 7, 2019

Choose a reason for hiding this comment

haruntuncay Apr 7, 2019

Choose a reason for hiding this comment

marci4 Apr 7, 2019

Choose a reason for hiding this comment

haruntuncay commented Apr 7, 2019 • edited Loading

marci4 commented Apr 7, 2019

haruntuncay commented Apr 14, 2019

marci4 commented Apr 16, 2019

marci4 commented Apr 17, 2019

haruntuncay commented Apr 17, 2019

marci4 commented Apr 17, 2019

haruntuncay commented Apr 17, 2019

marci4 commented Apr 17, 2019

haruntuncay commented Apr 17, 2019 • edited Loading

haruntuncay commented Apr 20, 2019

marci4 commented Apr 20, 2019

haruntuncay commented Apr 22, 2019

haruntuncay commented May 7, 2019

marci4 commented May 7, 2019

NoahAndrews commented May 7, 2019 • edited Loading

Canop commented Jan 8, 2020

marci4 commented Jan 8, 2020

Canop commented Jan 8, 2020

marci4 commented Mar 16, 2020

haruntuncay commented Mar 17, 2020

marci4 commented Mar 17, 2020

haruntuncay commented Mar 9, 2019 •

edited by marci4

Loading

haruntuncay commented Mar 17, 2019 •

edited

Loading

haruntuncay commented Apr 7, 2019 •

edited

Loading

haruntuncay commented Apr 17, 2019 •

edited

Loading

NoahAndrews commented May 7, 2019 •

edited

Loading