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

Added a generic extension mechanism and support for RFC7692 (permessage-deflate) #29

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

antipodite
Copy link

@antipodite antipodite commented Oct 12, 2020

Hi, I've been working on support for permessage-deflate in hunchensocket. I'm submitting this PR so you can comment on what I've done so far and see if you think anything needs to be done differently. I've tested on Chrome and Firefox, basic permessage-deflate works. I'm planning to add support for the other Sec-websocket-extension parameters in the RFC too e.g. max_window_bits. I've added 3 dependencies: the deoxybyte-gzip zlib cffi wrapper (I couldn't get flushing and sliding window maintenance to work with pure CL deflate implementations), trivial-garbage for implementation independent cleanup of the Zlib foreign data structures, and str because I'm lazy (this could be removed).

I tried to implement the extension framework in a generic way so it can support both non-standard extensions and user-defined extensions, like logging frames and messages in a human readable way.

A quick overview of how it works:

  • Extensions are defined as subclasses of WEBSOCKET-EXTENSION
  • The symbols of the extension classes along with any required headers and parameters to make-instance are passed to
    a websocket acceptor when it is created
  • Every time a client is created for a request the extensions passed to the acceptor are instantiated and passed into the client
  • There are 4 generics to do extension related processing dealing with ingoing and outcoming frames and messages. These are called in succession for each extension instance on the client, for incoming frames in handle-frame and outgoing frames/messages in the send-message function. So you specialise these for any extensions you define to carry out any processing that is needed on frames or messages.

I made a few changes to existing functions and methods to allow manipulation of frames etc, but all tests still pass, the existing behaviour has not changed in absence of extensions

This is my first time working on someone else's code so any feedback would be greatly appreciated

Chur

Isaac

- Implemented alpha stage extension interface
- Implemented some compression/decompression routines using different
  libs. Pre-alpha
- Modified the Zlib ffi wrapper to support preserving the LZ77 window
  and Z_SYNC_FLUSH
- Modified FRAME class to allow for reading and setting the extension
  bits within the header byte
@joaotavora
Copy link
Owner

Thank you very much, Isaac. I'm interested in this, but this is quite a large PR and It'll take some time to review. Are you sure there aren't CFFI/zlib alternatives in quicklisp already?

@antipodite
Copy link
Author

Thank you very much, Isaac. I'm interested in this, but this is quite a large PR and It'll take some time to review. Are you sure there aren't CFFI/zlib alternatives in quicklisp already?

The problem with the alternatives (e.g. salza2, chipz, zlib) is that the mechanism for implementing the extension as outlined in the RFC is closely tied to the reference implementation of Zlib, which is the C library. For example, the RFC requires that the server and client must preserve the "sliding window" compression buffer used to compress and decompress messages. I'm not familiar enough with the deflate algorithm to be able to mimic the way Zlib does this with the pure C alternatives. It's the same with preparing the compressed bytes to be sent, it's trivial using Z_SYNC_FLUSH but would require much more work with the CL alternatives.

Happy to provide more info if you need it.

@antipodite
Copy link
Author

Managed to have a look?

@joaotavora
Copy link
Owner

No sorry. I'm so very business lately. Thanks for bumping this, I'll bump it on my queue. I'm going to do some Websocket related work soon, which might be the kicker to have a good look at this.

@hanshuebner
Copy link
Collaborator

@antipodite Are you still interested in this?

@@ -21,8 +21,8 @@
;; `hunchensocket:*websocket-dispatch-table*` works just like
;; `hunchentoot:*dispatch-table*`, but for websocket specific resources.

(defvar *chat-rooms* (list (make-instance 'chat-room :name "/bongo")
(make-instance 'chat-room :name "/fury")))
(defparameter *chat-rooms* (list (make-instance 'chat-room :name "/bongo")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably makes sense, but should be outside this PR, or at least in a separate cleanup commit, no?

@@ -48,9 +48,13 @@
;; just like `hunchentoot:acceptor`, and you can probably also use
;; `hunchensocket:websocket-ssl-acceptor`.

(defvar *server* (make-instance 'hunchensocket:websocket-acceptor :port 12345))
(defparameter *server* (make-instance 'hunchensocket:websocket-acceptor
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise here.


(unless (hunchentoot::acceptor-listen-socket acceptor) ; should be
(unless (hunchentoot::acceptor-listen-socket *server*) ; should be
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto. Does this demo.lisp even compile? Perhaps it doesn't :-D


(asdf:defsystem :hunchensocket-tests
:description "Tests for Hunchensoket"
:description "Tests for Hunchensocket"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

into a cleanup commit or separate PR

@@ -1,7 +1,7 @@
(in-package :hunchensocket)

(define-constant +websocket-magic-key+
"258EAFA5-E914-47DA-95CA-C5AB0DC85B11"
"258EAFA5-E914-47DA-95CA-C5AB0DC85B11"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto: cleanup

@joaotavora
Copy link
Owner

@antipodite Are you still interested in this?

It certainly looks very good, if there tests it would be even better. Also my Git commit message OCD kicks in.

@antipodite
Copy link
Author

Yeah I guess I'm still interested but my PhD is taking up most of my time at the moment. It's kind of a while ago I'll have to refresh my knowledge of CL, but could commit to adding some tests in the next few months. My defense is getting uncomfortably close so that's the best I can do

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

Successfully merging this pull request may close these issues.

3 participants