Implement Compression Extensions

[garyburd]

Draft RFC: http://tools.ietf.org/html/draft-ietf-hybi-permessage-compression-17

Work remaining:

• Add fields to Dialer for specifying compression options.

• Add fields to Upgrader for specifying compression options.

• Add compression negotiation Upgrader.

• Add compression negotiation to Dialer.

• Add function to enable/disable write compression:

  // EnableWriteCompression enables and disables write compression of
  // subsequent text and binary messages. This function is a noop if
  // compression was not negotiated with the peer.
  func (c *Conn) EnableWriteCompression(enable bool) {
         c.enableWriteCompression = enable
  }
  

[cenkalti]

Is there anyone working on this?

[garyburd]

@cenkalti, not yet, but soon.

[fancycode]

Has there been any progress on this?

[garyburd]

There has not been progress on this.

[fancycode]

Ok, I will probably have something ready for a first review early next week.

[garyburd]

@fancycode Please give an overview of your design before doing a large amount of work.

[fancycode]

Sure, I will write a couple of notes later today or tomorrow. For now progress is pretty much stuck with some missing functionality in "compress/flate" (https://code.google.com/p/go/issues/detail?id=7836) which prevents supporting compression for fragmented packages...

[arvenil]

https://code.google.com/p/go/issues/detail?id=7836 is marked as fixed now, any chance for update on this?

[cenkalti]

I do not plan to implement this feature.

[euforia]

Is this implementation in progress? If so where can I follow?

[garyburd]

@euforia The implementation is not in progress.

[gevans]

👍 We'd love to see this implemented and posted a $100 bounty on Bountysource.

[euforia]

I'll try to put it on my docket. Will keep you guys posted.

[allaud]

Guys, I have some progress on this. I'm developing Go-based game server, which uses websocket protocol (gorilla implementation). For sure I need compression support for my messages, so I implemented support for compression extension, but with some reservations:

• It uses lzma, not deflate, as we use lzma on our client side (it should me extremely easy to change compression algo)
• It does not use *max_window_bits options
• It does not implement context takeover

For the last two options I don't think it is a problem as these features are optional for protocol extension. Please let me know if you are interested and I'll prepare fork, PR or something else.

[euforia]

That is great news. I am definitely interested. I can probably add more given there's a starting point. Would you mind pointing me to the repo?

[allaud]

@euforia I'm about to extract the code from our server and prepare a PR

[euforia]

Awesome. I'll keep an eye out. Much appreciated.

[allaud]

@euforia @garyburd This is the commit: allaud@23f691f

I understand it is a bit rude, but I suggest to start discussion from this point.

[euforia]

@allaud: This is perfect though I think permessage-deflate extension name may need to be changed to something like permessage-lzma, as deflate uses the deflate algo. May confuse other clients. Also this would allow for permessage-lzma specific params that deflate may not have. Any thoughts?

[allaud]

@euforia LZMA and Deflate libs both have same interfaces, so it should be easy to replace lzma algorithm with deflate. So I don't think it is a good idea to use permessage-lzma as there is no client library that supports this kind of extension. The only two described options are permessage-deflate and perframe-deflate

[euforia]

@allaud:
I guess I should've been more clear - my bad. I was thinking more along the lines of extending the functionality to support both lzma and deflate (the client would be able to choose), since you've already got lzma working. Were you referring just to deflate?

[allaud]

@euforia good idea, will try to find some time to implement

[euforia]

@allaud: seems like the test cases are failing when trying to write a control frame.

[euforia]

@allaud - Thanx for the initial boiler plate code. The current code has provisions to add arbitrary compression.

I have a working server implementation with permessage-deflate which can be found here - euforia@8ae9644. It contains an example under examples/compression.

• Currently the extension as used in the example is the only supported server extension and options.
• The compression level is hard coded to 1 for now but should be a configurable option.
• context_takeover and max_window_bits have yet to be implemented.

The implementation most definitely has room for improvement and optimization (compression, buffers etc.).

@garyburd - Would you mind taking a look to verify it meets the design needs? The compression / decompression code is in the compression.go file in an effort to be minimally invasive. Currently the logic is called in ReadMessage/WriteMessage but I feel like NextReader/NextWriter should just return the current reader/writer wrapped in a flate.Reader/Writer. Let me know what you think.

[garyburd]

• Add function to enable / disable compression. Remove CompressedTextMessage and CompressedBinaryMessage.

  // EnableWriteCompression enables and disables write compression of subsequent text and 
  // binary messages.  This function is a noop if compression was not negotiated with the peer.
  func (c *Conn) EnableWriteCompression(enable bool) {
     c.writeCompressionEnabled = enable
  }
  

• NextReader and NextWriter should return a wrapped read/reader writer. Skip the small optimization in WriteMessage when compression is enabled.

• Update client and server handshake code to negotiate compression.

• Deflate is the only compression algorithm specified in the RFC. Remove scaffolding to support other compression algorithms.

• The autobohn tests for supported features must pass.

[allaud]

@euforia @garyburd i can show my version of code, it is patched already to support switching on compression, support compression header and other small features. The only problem we are on quite old version of the lib and it is hardly patched

[euforia]

@garyburd - thx for the feedback. I'll make the appropriate changes.
@allaud - Point me to the code. I'll take a look.

[euforia]

@garyburd: Here's the updated code as requested - euforia@4c4c22e

It passes all autobahn tests and contains code per your guidelines. There are a couple of points to consider:

• The EnableWriteCompression function is there but commented out as we will need a corresponding readCompressedMessage variable to handle selective message compression. I was thinking selective compression can be a separate feature/PR. Any thoughts?
• I'm not sure what you meant by "Skip the small optimization in WriteMessage..." so I've left that be.

If everything checks out I can submit a PR. Let me know.

[garyburd]

Here are some quick comments. I'll look at the code in detail later.

The Upgrader and Client types should have fields specific to this extension and use them in the negotiation. As it stands now, a client application can specify extensions not supported by the package.

All compression code should be written specifically for permessage-deflate.

The EnableWriteCompression function is there but commented out as we will need a corresponding readCompressedMessage variable to handle selective message compression. I was thinking selective compression can be a separate feature/PR. Any thoughts?

Selective compression is part of the protocol. The reader should support it.

[euforia]

@garyburd: Updated as requested - euforia@b99472f

[euforia]

@garyburd - Any thoughts or should I just do a pull PR?

[garyburd]

The Upgrader and Client types should have fields specific to this extension and use them in the negotiation. As it stands now, an application can specify extensions not supported by the package.

Use boolean variable to indicate that compression was negotiated instead of a string length.

Do not compress control messages written with NextWriter.

Can the read code be simplified to the following?

  var r io.Reader = messageReader{c, c.readSeq} 
  if messageIsCompressed {
       r = flat.NewReader(io.MultiReader(r, strings.NewReader("\x00\x00\xff\xff")
  }
  return frameType, r, nil

Write an io.Writer adaptor between the flat writer and underlying message writer to capture the last bytes written. There's no need to allocate a []byte and copy to it on every write.

Skip the optimizations in WriteMessage when compression is enabled:

func (c *Conn) WriteMessage(messageType int, data []byte) error {
    wr, err := c.NextWriter(messageType)
    if err != nil {
        return err
    }
    w, ok := wr.(messageWriter) 
    if !ok {
        _, err := wr.Write(data)
        if err != nil {
            return err
        }             
        return wr.Close()
    }
 ... continue with optimization for single frame message
}

The compression writer does not need to know about the final frame flag.

The compression writer should not implement ReadFrom. There's no optimization to be had.

[ThomasAlxDmy]

@euforia What's the status for the PR? Do you need help?

[euforia]

Sorry haven't had a chance to work on this. This is what I have so far - https://github.com/euforia/websocket/tree/compress

[brandoncole]

@euforia Things are looking really great. Would love to find a way for this to make it in @garyburd. This could really help us as well.

[garyburd]

@brandoncole I am waiting on requested changes.

[euforia]

@garyburd - I've updated the code as requested. Can you please take a look before I do some cleanup and submit an official PR? Particularly the optimization in WriteMessage. Let me know.

https://github.com/euforia/websocket/tree/compress2

[swynter-ladbrokes]

Hi @euforia I tried your compress2 branch, edited the chat example to use your version and changed the upgrader...

Works perfectly....

Until the first ping (MessageType 9) goes through...

panic: interface conversion: io.WriteCloser is websocket.messageWriter, not *websocket.FlateAdaptorWriter

goroutine 20 [running]:
panic(0x3a88a0, 0xc820100200)
    /usr/local/Cellar/go/1.6/libexec/src/runtime/panic.go:464 +0x3e6
github.com/euforia/websocket.(*Conn).WriteMessage(0xc8200f4000, 0x9, 0x631fc0, 0x0, 0x0, 0x0, 0x0)
    src/github.com/euforia/websocket/conn.go:551 +0x186
main.(*connection).write(0xc8200d6160, 0x9, 0x631fc0, 0x0, 0x0, 0x0, 0x0)
    src/github.com/euforia/websocket/examples/chat/conn.go:65 +0x1ad
main.(*connection).writePump(0xc8200d6160)
    src/github.com/euforia/websocket/examples/chat/conn.go:86 +0x2b0
created by main.serveWs
    src/github.com/euforia/websocket/examples/chat/conn.go:102 +0x220

This patch fixes the panic, and doesn't seem to break anything else the client seems to be pinging just fine

diff --git a/conn.go b/conn.go
index a7c5345..ead6d45 100644
--- a/conn.go
+++ b/conn.go
@@ -10,6 +10,7 @@ import (
        "encoding/binary"
        "errors"
        "io"
+ "fmt"
        "io/ioutil"
        "math/rand"
        "net"
@@ -547,26 +548,24 @@ func (c *Conn) WriteMessage(messageType int, data []byte) error {
        }

        if c.compressionNegotiated && c.writeCompressionEnabled {
-
-           fw := wr.(*FlateAdaptorWriter)
-           if _, err = fw.Write(data); err != nil {
-                   return err
+         if fw, ok := wr.(*FlateAdaptorWriter); ok {
+                 if _, err = fw.Write(data); err != nil {
+                         return err
+                 }
+                 return fw.Close()
                }
-           return fw.Close()
-
-   } else {
+         fmt.Println("Should be compressed but isn't...")
+ }
+ w := wr.(messageWriter)
+ if _, err = w.write(true, data); err != nil {
+         return err
+ }

-           w := wr.(messageWriter)
-           if _, err = w.write(true, data); err != nil {
+ // final flush
+ if c.writeSeq == w.seq {
+         if err = c.flushFrame(true, nil); err != nil {
                        return err
                }
-
-           // final flush
-           if c.writeSeq == w.seq {
-                   if err = c.flushFrame(true, nil); err != nil {
-                           return err
-                   }
-           }
        }

        return nil

Ignore my patch...

09, Should be compressed but isn't...
08, Should be compressed but isn't...

at which point the client (chrome) disconnects

[garyburd]

@euforia It's looking good, but there are some minor issues remaining. Can you submit a PR with your squashed changes and I'll take it from there?

[euforia]

Will do. Thanks.

[dvdplm]

@garyburd any updates on this? Let us know if we can help out in any way! :)

[garyburd]

I added code to support the feature in a87eae1. The CL description lists the remaining work. Interested parties should let me know if they want to take on any of that work.

[y3llowcake]

FYI I am interested in seeing this work completed and I have started to come up to speed with the internals of this library. If someone is already looking into this, or has further suggestions beyond the comments in a87eae1 please let me know.

[garyburd]

I updated the issue with a check list of work to do.

Work on the Conn type should be complete. When compression is negotiated, set the newCompressionWriter and newDecompressionReader fields to functions that wrap the underlying writer and reader with compression.

The functions compressNoContextTakeover and decompressNoContextTakeover implement no context takeover wrapper functions.

The parseExtensions function parses the extension header and should be use in the Upgrader negotiation.

[y3llowcake]

I'd like to solicit early feedback on the following approach: Only support server+client no context takeover at first. This limits the scope of my initial changes, and is a relatively simple change compared to full negotiation support.

The following new fields/methods would be exported from the package if this approach is taken:

• A boolean field called CompressionEnabled is added to Dialer and Upgrader.
• The EnableWriteCompression function is added to conn.

[garyburd]

Only support server+client no context takeover at first.

Yes. The first step is to add the negotiation required to use the existing no context takeover compression wrapper functions.

following new fields/methods would be exported from the package

The exported fields/methods look good.

In the long term, there should be a Conn method to that returns the negotiated compression options. I suggest holding off on that until it's worked out how to specify negotiation options in the Dialer and Upgrader.

[garyburd]

Closed by c09b72d.