New design for client -- server protocol for Snowflake
Right now we just send traffic. We could add some control messages or another layer for reliability/sequencing.
Activity
At moment, we're focusing on the proxy--broker protocol, but it is somewhat related. I'll assign it to myself since we'll cover it together with #29207 (moved)
Trac:
Owner: N/A to cohosh-
Here's a first stab at a very simple sequencing layer for snowflake: https://github.com/cohosh/snowflake/compare/sequencing
There is still a lot to do such as:
- send acknowledement packets when data has been received
- implement a timeout feature (which I will probably do by making
SnowflakeReadWriteran actualnet.Conn. - resend data chunks that have not been acknowledged
- implement a fixed sized window to avoid sending too much data
But I thought I'd put this out there sooner to get feedback since this is a big change
Trac:
Cc: cohosh to cohosh, phw, dcf, arlolra -
I'm going to ask for an incremental review on this one, mostly just to get another pair of eyes on what I've done and my next steps before I sink more time into going possibly the wrong direction: https://github.com/cohosh/snowflake/compare/sequencing
What I've done:
- Added a
ReadWriteCloserthat takes data to be sent and adds a header before passing it on to the Write function of the webrtc connection and removes the header in a Read on the other side. This is implemented as a common package because it's used by both the client and server - Wrote some tests for the package
- Send and acknowledgement packet at every read: https://github.com/cohosh/snowflake/commit/a7191b6b5ea4c9e58709c03fefc2dcd07571dc0f
Next steps:
- Implement a timeout (maybe making the
ReadWriteCloseranet.Connand implementingSetReadDeadlineand a fixed size window to stop sending packets before others have been acknowledged) - Make it backwards compatable. I'm thinking of doing this by having an extra field in the header that should be set to all zeros. If it's not zeros, we just forward the data as is. If it is zeros, we extract the header.
- Make the tests better. Right now I'm just reading and writing from a buffer, we should test with network connections
Trac:
Status: assigned to needs_review - Added a
-
Replying to cohosh:
- Make it backwards compatable. I'm thinking of doing this by having an extra field in the header that should be set to all zeros. If it's not zeros, we just forward the data as is. If it is zeros, we extract the header. Actually another way of doing backwards compatability is to make a new handler for the new version, or add an http header with some kind of versioning information.
Replying to cohosh:
I'm going to ask for an incremental review on this one, mostly just to get another pair of eyes on what I've done and my next steps before I sink more time into going possibly the wrong direction: https://github.com/cohosh/snowflake/compare/sequencing
High-level summary: splits the data stream into frames. Each frame is preceded by a 12-byte header consisting of seq, ack, and length fields. The receiver buffers an entire frame in memory, then accepts it if its sequence number exactly matches the expected sequence number, or drops it otherwise.
Overall the design looks good.
type snowflakeHeader struct { seq int ack int length int //length of the accompanying data (including header length) }The fields would be better as
uint32, because the size ofintmay vary (though I think you handled it correctly by always specifying 32 bits in serialization functions).lengthmay be better asuint16; that's plenty for one call toWriteand it avoids the risk that an implementation will try to buffer 4 GB in memory.IMO
lengthshould be exclusive of the header size. Otherwise you have to define what it means iflength< 12. (I believe the current code doesn't work in this case becauseheader.length - snowflakeHeaderLenunderflows.) The packets sent bysendAckshould havelength= 0.h.seq = int(binary.LittleEndian.Uint32(b[0:4])) h.ack = int(binary.LittleEndian.Uint32(b[4:8])) h.length = int(binary.LittleEndian.Uint32(b[8:12]))Big-endian is more usual for network protocols.
I'm not sure about the implementation of
Read:-
Should return
0here, notlength.bhasn't really been filled in with any de-serialized data yet.length, err := s.Conn.Read(b) if err != nil { return length, err } }}} -
This should have
b[:length]in place ofb: {{{ s.buffer = append(s.buffer, b...) }}} -
This order of operations (
Readthencopy) will lose any data remaining ins.outat EOF. It would be better to do thecopyfirst, and only whens.outis empty do aReadand possibly return an error. {{{ length, err := s.Conn.Read(b) if err != nil { return length, err } s.buffer = append(s.buffer, b...)n := copy(b, s.out) s.out = s.out[n:] if n == len(b) { return n, err } }}} I would prefer to start with an implementation of
Readthat is simpler, if less efficient. I am picturing something like this (untested): {{{ func NewSnowflakeReadWriter(sConn io.ReadWriteCloser) *SnowflakeReadWriter { pr, pw := io.Pipe() s := &SnowflakeReadWriter{ Conn: sConn, pr: pr, } go s.readLoop(pw) return s } func (s *SnowflakeReadWriter) readLoop(pw io.PipeWriter) { var err error for err == nil { // strip headers and write data into the pipe var header snowflakeHeader err = readHeader(s.Conn, &header) if err != nil { break } if header.seq == s.ack { _, err = io.CopyN(pw, s.Conn, int64(header.length)) } else { _, err = io.CopyN(ioutil.Discard, s.Conn, int64(header.length)) } } pw.CloseWithError(err) } func (s *SnowflakeReadWriter) Read(b []byte) (int, error) { // read de-headered data from the pipe return s.pr.Read(b) }
> * Implement a timeout (maybe making the ReadWriteCloser a net.Conn and implementing SetReadDeadline and a fixed size window to stop sending packets before others have been acknowledged) I agree that ultimately `SnowflakeReadWriter` should be a `net.Conn`, though I don't see why the read deadline should affect the sending of packets. > * Make the tests better. Right now I'm just reading and writing from a buffer, we should test with network connections [net.Pipe](https://golang.org/pkg/net/#Pipe) can be good for this.-
-
Trac:
Status: needs_review to needs_revision
Summary: New design for client -- proxy protocol for Snowflake to New design for client -- server protocol for Snowflake
Description: Right now we just send traffic. We need to add some control messages for communication between the client and proxy.to
Right now we just send traffic. We could add some control messages or another layer for reliability/sequencing.
-
Replying to dcf:
Replying to cohosh:
I'm going to ask for an incremental review on this one, mostly just to get another pair of eyes on what I've done and my next steps before I sink more time into going possibly the wrong direction: https://github.com/cohosh/snowflake/compare/sequencing
Thanks for the feedback! I've addressed most of the feedback so far here: https://github.com/cohosh/snowflake/tree/sequencing
The fields would be better as
uint32, because the size ofintmay vary (though I think you handled it correctly by always specifying 32 bits in serialization functions).lengthmay be better asuint16; that's plenty for one call toWriteand it avoids the risk that an implementation will try to buffer 4 GB in memory. Fixed in c102894IMO
lengthshould be exclusive of the header size. Otherwise you have to define what it means iflength< 12. (I believe the current code doesn't work in this case becauseheader.length - snowflakeHeaderLenunderflows.) The packets sent bysendAckshould havelength= 0.Fixed in 02bfa1b
Big-endian is more usual for network protocols.
Fixed in 7566cb
I'm not sure about the implementation of
Read: I would prefer to start with an implementation ofReadthat is simpler, if less efficient. I am picturing something like this [...] I like this implementation a lot more. Fixed in b6fb987 and 9c1b12fI'm going to start on implementing fixed size send windows and cleaning up the unit tests a bit.
