Want to implement rfc7692, but writer side can not be implemented

[smith-30]

Hi,

I'd like to use the context-takeover mechanism defined in rfc7692.
I forked and developed it and I could implement the reader side.
This [branch] (https://github.com/smith-30/websocket/tree/feature/upgrade_writer) is the newest.

However, I am in trouble because I can not implement the writer.

Implementation I'm thinking

Attempting to implement context-takeover by attaching flateWriteWrapper to Conn struct.

In flateWriteWrapper, attach flat.Writer called with flat.NewWriterDict.
https://github.com/smith-30/websocket/blob/ee46f8548a106a02264f711a1838887fd3cf58cf/conn.go#L518-L536
I do not want to make flateWriter every time I make a call.
Because performance is very poor.
Not using Pool is because GC may clean it without permission.
Avoid the window of flateWriter disappearing and inconsistency with reader side.

However, in my implementation I can not initialize the truncWriter passed to flateWriter.
In the second execution, truncWriter has state and fails in compress processing.
I'd like to reset the state of truncWriter after compression
Is there a good way to do it?

I am sorry for my poor English.
It would be extremely helpful If you review my implementation..

[smith-30]

note

The client of context-takeover uses python's one.
https://github.com/aaugustin/websockets

I will post code that I am using for debugging if necessary.

[garyburd]

The code in conn.go is written to support context takeover. When context takeover is used, the compression factory functions (newDecompressionReader, newCompressionWriter) maintain per-connection state through a closure or a method value. This state will include the flate.Writer or flate.Reader for the connection. A method value implementation will look something like this:

type struct contextTakeoverWriterFactory {
     flateWriter flate.Writer
     // and other fields
}

func (f *contextTakeoverWriterFactory) newCompressionWriter(w io.WriteCloser, level int) io.WriteCloser  {
    // wedge w under f.flateWriter
    // return wrapper around f.flateWriter
}

The Upgrader.Upgrade and DIaler.Dial methods will setup the factory and assign a method value to the factory function fields:

  var f contextTakeoverWriterFactory
  f.flateWriter = ... // create flate.Writer for this conneciton
 // Set other fields
  c.newCompressionWriter = f.newCompressionWriter

The truncWriter, flateWriteWrapper and flateReadWrapper types are written to support no context takeover. A new set of types will be needed to support context takeover.

[garyburd]

Here's more of the sketch for implementing the context takeover writer. The truncWriter type can be used as is.

type struct contextTakeoverWriterFactory {
     fw *flate.Writer
     tw truncWriter
}

func (f *contextTakeoverWriterFactory) newCompressionWriter(w io.WriteCloser, level int) io.WriteCloser  {
    f.tw.w = w
    f.tw.n = 0
    return flateTakeoverWriteWrapper{f}
}

type flateTakeoverWriteWrapper struct {
    f *contextTakkeoverWriterFactory
}

func (w *flateTakeoverWriteWrapper) Write(p []byte) (int, error) {
	if w.f == nil {
		return 0, errWriteClosed
	}
	return w.f.fw.Write(p)
}

func (w *flateTakeoverWriteWrapper) Close() error {
	if w.f == nil {
		return errWriteClosed
	}
       f := w.f
       w.f = nil
       err1 := f.fw.Flush()
	if f.tw.p != [4]byte{0, 0, 0xff, 0xff} {
		return errors.New("websocket: internal error, unexpected bytes at end of flate stream")
	}
	err2 := f.tw.w.Close()
	if err1 != nil {
		return err1
	}
	return err2
}

The setup code is:

var f contextTakeoverWriterFactory
f.fw = flate.NewWriter(&f.tw, level)  // level is specified in Dialer, Upgrader
c.newCompressionWriter = f.newCompressionWriter

I am not very happy with the type names I am suggesting here, but I don't have any better suggestions at the moment. The name contextTakeoverWriterFactory sounds like it's from a Java program.

[smith-30]

Thank you very much.
I was able to successfully implement Writer's side.
Especially your sketches helped me.

Initially, I thought that it was bad that I could not grasp the relationship between the dictionary used by flate.Writer and the window.
With the flate.Writer only support for the sliding window mechanism, no-context-takeover was doing Reset to not use it.
I was able to notice it with implementation hints.

Since the Reader side also implements forcible implementation, I think to rewrite it to Factory pattern.

[smith-30]

I could create readers and writers implementing context-takeover.
Is there any possibility that changes will be merged into your repository if I issue PR?

If you do not plan to support context-takeover in your repository, I will develop it as Fork.
I think that it is difficult that my implementation strictly does not follow RFC (window_bits can not be chosen, it only returns 15)

Like this, if compress true and context-takeover true

permessage-deflate; server_max_window_bits=15; client_max_window_bits=15

[smith-30]

Performance of Write is equivalent to that of NoContext

func BenchmarkWriteWithCompression(b *testing.B) {
		w := ioutil.Discard
		c := newConn(fakeNetConn{Reader: nil, Writer: w}, false, 1024, 1024)
		messages := textMessages(100)
		c.enableWriteCompression = true
		c.newCompressionWriter = compressNoContextTakeover
		b.ResetTimer()
		for i := 0; i < b.N; i++ {
			c.WriteMessage(TextMessage, messages[i%len(messages)])
		}
		b.ReportAllocs()
}

^BenchmarkWriteWithCompression$

goos: darwin
goarch: amd64
pkg: github.com/smith-30/websocket
BenchmarkWriteWithCompression-4       300000          4388 ns/op         164 B/op          3 allocs/op
PASS
ok      github.com/smith-30/websocket   1.370s
Success: Benchmarks passed.

func BenchmarkWriteWithCompressionOfContextTakeover(b *testing.B) {
		w := ioutil.Discard
		c := newConn(fakeNetConn{Reader: nil, Writer: w}, false, 1024, 1024)
		messages := textMessages(100)
		c.enableWriteCompression = true
		c.contextTakeover = true
		var f contextTakeoverWriterFactory
		f.fw, _ = flate.NewWriter(&f.tw, 2) // level is specified in Dialer, Upgrader
		c.newCompressionWriter = f.newCompressionWriter
		b.ResetTimer()
		for i := 0; i < b.N; i++ {
			c.WriteMessage(TextMessage, messages[i%len(messages)])
		}
		b.ReportAllocs()
}

^BenchmarkWriteWithCompressionOfContextTakeover$

goos: darwin
goarch: amd64
pkg: github.com/smith-30/websocket
BenchmarkWriteWithCompressionOfContextTakeover-4      500000          3190 ns/op          56 B/op          2 allocs/op
PASS
ok      github.com/smith-30/websocket   1.647s
Success: Benchmarks passed.

[garyburd]

Is there any possibility that changes will be merged into your repository if I issue PR?

Yes

I think that it is difficult that my implementation strictly does not follow RFC (window_bits can not be chosen, it only returns 15)

The RFC does not require the endpoint to support max_window_bits.