Allow to accept websocket extensions

[albertas]

Also accept permessage-deflate, permessage-bzip2 and permessage-snappy
compression extensions by default if client requests for them.
Compression/decompression of the messages is taken care of by autobahn
package.

[albertas]

Fixes #326

[albertas]

Hey @carltongibson, could you please take a look at this PR. I could update it as needed.

[carltongibson]

Hi @albertas Thanks for this. I'm going to focus on the Channels release and then I'll cut back to do a release here!

[albertas]

Hi @carltongibson, could I get an update/feedback for this PR? I see that Daphne v3 was already released. Is there any chance for releasing this PR in near future?

[carltongibson]

Hi @albertas — sorry for the pause. Only so much bandwidth.

If you can rebase we'll look to get in. Thanks! 👍

[benrudolph]

If I'm understanding correctly, Daphne doesn't support websocket extensions as-is and this PR would add support for that?

[carltongibson]

@benrudolph If you wanted to pick this up and rebase it, I'd be happy to have a look.

[benrudolph]

alrighty, i'll take a look

[albertas]

Well, merge conflicts were trivial: only for added imports. I have solved them using web editor in no time.
Looking forward for your reviews.

[carltongibson]

So @benrudolph — does this allow you to do what you're after?

[benrudolph]

thanks, yes. i realize that this just enables adding extension headers to support compression, not compression itself, but this does help. channels doesn't really provide the capability to compress out of the box, but it's certainly a no-go if daphne ignores the headers. in any case, yes, this is great, thanks!

[carltongibson]

Ok, thanks for the feedback @benrudolph. If you want to discuss what else you'd need in a Discussion on Channels that would be good, but we'll look to get this in here in the meantime.

[benrudolph]

Thanks appreciate it

[albertas]

@benrudolph Well, Daphne uses Autobahn package underneath, which has implementation for various compressions. This PR both enables to accept any kind of headers and enables permessage-deflate, permessage-bzip2 and permessage-snappy compressions. permessage-deflate compression should be fully working with this PR, but it still needs testing in real environment.

[benrudolph]

oh amazing! 🔥 🔥 that's a huge win

[benrudolph]

@carltongibson anything we can do to get this puppy in?

[carltongibson]

@benrudolph Not really. You could install it with pip, test it, report back.

On schedule: We had a channel_redis release this week. Next up is Channels itself. Then I will look back to Daphne. Sorry if that's slow for you, but it's just me so it sometimes takes a while. 🙂

[benrudolph]

sounds good. wow! you're maintaining those 3 libraries solo?? that's simultaneously incredible and a little terrifying 👏

[carltongibson]

It’s mostly terrifying. 😃 (folks do help, but input welcome!)

[albertas]

bump

[albertas]

@carltongibson How could I help to push this one out? I will test this PR by writing simple django project example with permessage-deflate compression enabled. I could also update/add channels documentation. Also I could add additional tests. Just let me know whats needed.

[carltongibson]

Hi @albertas — Yes... — my issue is in getting to it. I'm pretty sure it's correct, but I need to review, and that involves digging into the spec and the underlying Autobahn implementation, and I just haven't got to that yet.

I guess a couple of choice links here pointing me where to look would speed that up. (Silly as that sounds...)

Then, do we need any additions to the README here?

[carltongibson]

@albertas — Just to say more positively, it's been summer, but let's get a release done in September, with this in it.

(By any I mean some — if I'm a user, what helps me leverage this? —  Probably those links... 🤔)

Thanks!

[albertas]

@carltongibson here are several links I found:

• WebSocket protocol specification: https://tools.ietf.org/doc/html/rfc6455
• permessage-deflate specification: http://tools.ietf.org/html/draft-ietf-hybi-permessage-compression
• Autobahn docs about permessage compression: https://github.com/crossbario/autobahn-python/blob/fd5530cecff0be210b148bddf1e984ed31d2863c/docs/work/compression.rst
• permessage-deflate implementation in Autobahn package: https://github.com/crossbario/autobahn-python/blob/fd5530cecff0be210b148bddf1e984ed31d2863c/autobahn/websocket/compress_deflate.py#L70
• Example project to test Autobahn permessage_deflate compression: https://github.com/crossbario/autobahn-python/tree/master/examples/twisted/websocket/echo_compressed

[carltongibson]

Super, thanks @albertas — FYI I'm picking this up at the end of this week. Thanks for your input! 🎁

[carltongibson]

Hey @albertas — another update — I'm looking at this. Didn't make the Sept target, but Daphne is now my active thing. Thanks for your patience.

[albertas]

@carltongibson Is there a way I could help? 🙂

[carltongibson]

Hey @albertas. Yes. Sorry. life.

I haven't had any bandwidth at all for anything in the last few weeks.

This issue is at the very top of my list, and will be the first I look at. (I've half looked and I think it's fine, but just need a cycle to confirm that to myself.)

My next target is a release for Daphne.

If you'd like to look at the other issues and PRs and think what's addressable for a release, that would be awesome. It is just me, so an extra hand in the medium term would make things more sustainable.

[carltongibson]

@albertas Just to give some guidance on that — in case you take me up on it 😄 — I'm thinking to update the dependencies, check the Windows compatibility with the latest twisted, look at #319 properly (despite the Django auto-reloader issues) and think about dropping PY36 and PY37 (since it's not clear task cancelling is being handled properly...) — There's plenty there to begin on.

[pbhd]

Is there any progress with this pull-request? We are enabling permessage-deflate with a strange hack by searching the WebSocketFactory in the gc.get_objects()... Having this work out of the box would really be nice

[albertas]

@carltongibson Is there any chance of getting this PR forward? I could assist if needed.

[adityabansalx]

@albertas @carltongibson Any update on this pull request. it would be great to see this live.

[carltongibson]

Yes, this is on my list for the next set of releases.

[adityabansalx]

Yes, this is on my list for the next set of releases.

@carltongibson you are a life saver !

[adityabansalx]

Hi, @carltongibson, any update on this feature? seeing it live would be really nice.

[carltongibson]

It's coming. Please don't post "any updates" type comments. Thanks.

[carltongibson]

Sorry, clicked the wrong button. Didn't mean to close it. 🤦‍♀️

[MABBTT]

I'm a hobbyist, new to programming and git, is there anything a novice such as myself can do to help with this?

I've been attempting to implement permessage-deflate in an application that utilizes channels and daphne. My understanding is that autobahn supports headers via ASGI spec 2.1 and has support for multiple message compression methods and channels now also supports headers with django/channels#2002. Therefore, WebSocket message compression with daphne/channels is dependent on this pull request.

Edit: I have now tested this pull request merged with a fork of the current 'main' branch within my application which utilizes channels. I have observed via wireshark that it does indeed result in per message compression whereas before this was not possible with channels and daphne, however, it behaved in ways I did not expect with channels.

In the current PR, my understanding is that daphne automatically accepts any extension that exists in autobahn PERMESSAGE_COMPRESSION_EXTENSION, the client offer and:

        websocket_permessage_compression_extensions=(
            "permessage-deflate",
            "permessage-bzip2",
            "permessage-snappy",
        ),

Now that channels accepts headers, self.accept(headers=[(b"Sec-WebSocket-Extension",b"permessage-deflate; client_max_window_bits")]) is possible but doing so results in a situation where the response header will include "Sec-WebSocket-Extension" more than once in the opening handshake which causes a conflict with autobahn and causes the handshake to fail. At least in my environment there was no error message that bubbled up through to channels to indicate the error which I think is a concern.

#autobahn/websocket/protocol.py
            # Sec-WebSocket-Extensions
            #
            self.websocket_extensions = []
            if 'sec-websocket-extensions' in self.http_headers:
                if http_headers_cnt["sec-websocket-extensions"] > 1:
                    return self.failHandshake("HTTP Sec-WebSocket-Extensions header appears more than once in opening handshake request")
                else:
                    # extensions requested/offered by client
                    #
                    self.websocket_extensions = self._parseExtensionsHeader(self.http_headers["sec-websocket-extensions"])

https://github.com/crossbario/autobahn-python/blob/7bc85b34e200640ab98a41cfddb38267f39bc92e/autobahn/websocket/protocol.py#L2866

Now that channels supports headers, are this PR's changes to daphne/server.py needed and desirable or would it be preferable to simplify the PR to only add support for headers in daphne/ws_protocol.py and place the responsibility solely on the upstream application (channels) for determining what if any handshake response headers are to be specified?

I am happy to try to help push this forward, but as you likely can tell from my writing and the fact that this is my first GitHub comment, I am far out of my depth here. Thanks.