ECN support #365
ECN support #365
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #365 +/- ##
==========================================
+ Coverage 81.67% 83.99% +2.31%
==========================================
Files 37 40 +3
Lines 3564 3174 -390
==========================================
- Hits 2911 2666 -245
+ Misses 521 374 -147
- Partials 132 134 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
JoTurk
left a comment
JoTurk
left a comment
There was a problem hiding this comment.
This is exciting, thank you! And sorry for all the comments, I tried to avoid anything super nitpicky. adding new APIs is always a bit hard .
It'd be great to also have tests covering the attributes path.
| func (b *Buffer) Write(buff []byte) (n int, err error) { //nolint:cyclop | ||
| return b.WriteWithAttributes(buff, nil) | ||
| } |
There was a problem hiding this comment.
we shouldn't change the normal write path, this is a behavior change, and it will break applications that depend on this logic.
There was a problem hiding this comment.
Hi Joe, I will address the comments one by one. Thank you for your great feedback.
No it will not break it. Whether you write the a packet with attributes or no attributes (nil in this case), an additional byte segment will be stored in the buffer. The buffer saves the packet length as two bytes first, and then the packet itself. With my changes, every written packet will have two segments. The first segment is like before, so two bytes for length and the rest for the packet. The second segment stores the attributes, first the attributes length (two bytes for that) and the attributes payload. When reading from buffer, regardless of method, both segments are consumed from the buffer (the packet and the payload itself). When writing into the buffer, regardless of having attributes present or not, a segment of size 2+ (2 for the segment length) will be stored in the buffer. Now if the attribtues are nil, only the empty segment's length (=2) will be stored.
Also, If it were to break anything, I couldn't have my sender and receiver working. However, they work fine and all the ICE candidate exchange and RTP/RTCP exchange work properly.
There was a problem hiding this comment.
We're not the only users of pion/transport, even if receive and send works for us. If a user writes 2 bytes using Write and now it writes 4 bytes. this is a breaking behavior, even if the API itself didn't change.
We can add like a private flag or something hasAttributes.
There was a problem hiding this comment.
No you're not on the right track. What does a user of PacketBuffer see? They are concerned with writing a packet properly to the buffer, and reading one packet properly from the buffer. The internals of the buffer are not the user's business.
In your case, we write 2 bytes (2 bytes only for the payload of the packet) to the buffer but:
- We return 2 to the user (only the length of payload's bytes written to the packet buffer) so the user does not even understand more bytes are stored actually
- When we read from the buffer, we consume all the 4 bytes written and still tel the user we read 2 bytes so no behavior change here either.
P.S. Is your concern the method Size? I didn't pay attention, yes the behavior of Size shouldn't change you're right. I thought it's internal to the buffer. I will fix this.
There was a problem hiding this comment.
This is not correct, because in packetio.Buffer the internal byte count directly affects observable behavior through:
- Size()
- SetLimitSize()
- SetLimitCount()
- backpressure via Write() returning ErrFull when the buffer is full, now we have extra bytes.
- how grow() decides the allocation size
- how many packets fit before being dropped/rejected
So even if Write() returns the same n and Read() returns the same n, the extra internal bytes materially change the buffer's behavior, it's not just about size.
There was a problem hiding this comment.
Now I see your point. Thanks for your patience. Yes you're right. I checked the methods that call Size() in other repos. It seems like they are not using it so it's almost internal method. For SetLimitSize(), it seems like the callers are not that exact in the set size as they set the size to 100*1000 for example, so the additional 2 bytes practically change nothing in the behavior. Now If you say we should maintain the exact exact same behavior I will do this.
There was a problem hiding this comment.
Thank you, and again i'm sorry, introducing / changing APIs is always hard, and pion/transport touches every library we have.
packetattributes.go
Outdated
| // for serializing | ||
| Marshal() []byte | ||
|
|
||
| // for de-serializing. The bytes will be copied into the returned buffer | ||
| GetBuffer() []byte |
There was a problem hiding this comment.
Maybe marshal/Unmarshal or Bytes/Scratch or any common serialize/de-serialize naming convention that is common in Go.
There was a problem hiding this comment.
Yes yes I'm familiar with that. My idea was, GetBuffer returns the attributes buffer so bytes can be directly copied into the PacketAttributes' buffer instead of copying first into a temp buffer and then calling UnMarshall. The reason is, through a packet's journey, a packet is written/read to the packet buffer multiple times.
If you think there's a better option or UnMarshall is a good enough name, I'm fine with that.
There was a problem hiding this comment.
I don't like marshal/unmarshall personally, but generally it's better to follow the naming conventions. as long as you also have marshal.
packetattributes.go
Outdated
|
|
||
| const attrMaxLen = 1 | ||
|
|
||
| type ECN uint8 |
There was a problem hiding this comment.
It might be better if transport's packet-attributes stayed generic, leaving ECN and other metadata details to higher-level application logic.
Because the way i see it is that ECN is a semantics not mechanism, and transport is just a thin layer / helper library, and it shouldn't care about details.
this comment also extends to ECN and WithECN..
packetattributes.go
Outdated
| } | ||
|
|
||
| type PacketAttributes struct { | ||
| buffer [attrMaxLen]byte |
There was a problem hiding this comment.
I think we should make the attribute size dynamic and add a byte to determine how many attributes are there so we don't have to make attributesv2 in the future.
There was a problem hiding this comment.
No we don't really need to make attributesv2 in the future. If we were to add more fields to the PacketAttributes we can just do it. The packet buffer is agnostic of PacketAttributes' buffer length and it can work with any variable sized PacketAttributes.
There was a problem hiding this comment.
My comment was more about that we don't need to touch pion/transport to add new attributes, we should make it generic, IMO: no attrMaxLen, no ECN aware. just generic attributes reader and writer.
packetattributes.go
Outdated
|
|
||
| func NewPacketAttributes() *PacketAttributes { | ||
| p := &PacketAttributes{} | ||
| p.Reset() |
| type NetConnSocket interface { | ||
| net.Conn | ||
|
|
||
| ReadWithAttributes(p []byte, attr *PacketAttributes) (n int, err error) |
There was a problem hiding this comment.
I think it would be better if we unify attribute types in the APIs.
There was a problem hiding this comment.
I think having a generic packet attributes type with variable buffer length would address this comment right?
There was a problem hiding this comment.
yeah a generic packet attributes will be cool, good idea.
| assert.NoError(err) | ||
| assert.Equal(2, n) | ||
| assert.Equal(4, buffer.Size()) | ||
| assert.Equal(6, buffer.Size()) |
There was a problem hiding this comment.
This is related to the comment about buffer.Write. we shouldn't change the test, and size shouldn't count attributes imo (not 100% sure).
There was a problem hiding this comment.
I explained that buffer's write writes two segments in the buffer per packet. So this has to change. Also if you're not confident that it works, I have added a test at the end where I write some packets without attributes and read the same packets in order with attributes. It doesn't break any functionality.
P.S. yes Size is a public facing method so its behavior shouldn't change. I will update this.
There was a problem hiding this comment.
It's not about that it's working :) we shouldn't change how Size works for the users that didn't use attributes :)
packetio/buffer.go
Outdated
| b.advanceHead(count) | ||
|
|
||
| // read the attributes segment | ||
| aLen := b.getSegmentLength() | ||
| if aLen > 0 { | ||
| if attr != nil { | ||
| b.writeToInputBuffer(attr.GetBuffer(), aLen) | ||
| } | ||
| b.advanceHead(aLen) | ||
| } |
There was a problem hiding this comment.
there is no guarantee len(GetBuffer()) >= aLen, if someone implements their own attributes interface in the future or something breaks down the line, this code will be unsafe.
There was a problem hiding this comment.
Yes you're right. I missed this one.
packetio/buffer.go
Outdated
|
|
||
| if attr != nil { | ||
| // store the attributes buffer itself | ||
| b.writeFromInputBuffer(attr.Marshal()) |
There was a problem hiding this comment.
Can we make it so we call attr.Marshal once?
connsocket.go
Outdated
| func NewNetConnToNetConnSocket(conn net.Conn) *NetConnToNetConnSocket { | ||
| return &NetConnToNetConnSocket{conn: conn} | ||
| } | ||
|
|
||
| // ReadWithAttributes reads from the underlying connection and ignores attributes. | ||
| func (p *NetConnToNetConnSocket) ReadWithAttributes(b []byte, _ *PacketAttributes) (int, error) { | ||
| return p.conn.Read(b) | ||
| } | ||
|
|
||
| // Delegate net.Conn methods to the underlying connection. | ||
| func (p *NetConnToNetConnSocket) Read(b []byte) (int, error) { return p.conn.Read(b) } | ||
| func (p *NetConnToNetConnSocket) Write(b []byte) (int, error) { return p.conn.Write(b) } | ||
| func (p *NetConnToNetConnSocket) Close() error { return p.conn.Close() } | ||
| func (p *NetConnToNetConnSocket) LocalAddr() net.Addr { return p.conn.LocalAddr() } | ||
| func (p *NetConnToNetConnSocket) RemoteAddr() net.Addr { return p.conn.RemoteAddr() } | ||
| func (p *NetConnToNetConnSocket) SetDeadline(t time.Time) error { return p.conn.SetDeadline(t) } | ||
| func (p *NetConnToNetConnSocket) SetReadDeadline(t time.Time) error { return p.conn.SetReadDeadline(t) } | ||
| func (p *NetConnToNetConnSocket) SetWriteDeadline(t time.Time) error { | ||
| return p.conn.SetWriteDeadline(t) | ||
| } | ||
|
|
||
| type PacketConnToPacketConnSocket struct { | ||
| conn net.PacketConn | ||
| } | ||
|
|
||
| func NewPacketConnToPacketConnSocket(conn net.PacketConn) *PacketConnToPacketConnSocket { | ||
| return &PacketConnToPacketConnSocket{ | ||
| conn: conn, | ||
| } | ||
| } |
There was a problem hiding this comment.
can you please explain why we need these two structs and why we need to make them public?
I also think the name of PacketConnToPacketConnSocket can be better.
There was a problem hiding this comment.
The answer is not in transport but in other repos like srtp and webrtc. The issue is, the socket is abstracted with either net.Conn or net.PacketConn in the code. For example take mux.
mux implements net.Conn so it has a method Read or Write (just like a socket) but I needed to tell my mux to ReadWithAttributes. This is why these two structs are defined.
There was a problem hiding this comment.
Can't this just be a helper-cast function that simply returns an interface, err without exposing a new public struct?
There was a problem hiding this comment.
Do you mean something like?
var conn net.Conn
if s, ok := conn.(*NetConnSocket); ok != nil {
// some code
}yes this is doable. I'm not much experienced in Go, but I just learned this for the changes I made, so initially I didn't think of this.
|
Thanks @joeturki for your comments. I will work on a revised version and will come back soon. |
|
@joeturki What do you think about the new changes? You mentioned the problem with methods
For |
|
@penhauer Hey, first of all, we're all just trying to have fun here, so please let's just avoid non-technical phrasing, this is the second time you did this :) Our only requirement in Pion is that we never change existing behavior (API or logic) unless we have a major upgrade, other than that everything is a fair game. and as I said, we can meet that easily by adding a private flag for buffers constructed with attributes, or by keeping an extra buffer for attributes (nil by default). This keeps everything fully backward-compatible. This isn't an internal library so the network behavior doesn't matter here, because again, this is a public facing library, and with non-breaking changes we typically don't have to change unit tests for public facing functions. |
|
Sorry I didn't mean that in a bad way but your comment is fair. How about introducing a new |
|
@penhauer Sorry i got sick (fever) and lost track of this, I'm just back as active now, I think we can keep the same
|
Description
For supporting ECN, it was needed to change the PacketBuffer so it can contain attributes like ECN or TOS. The new PacketBuffer can store packets with additional variable length attributes.
For the rest of pion, it was needed to add a ReadWithAttributes/WriteWithAttributes method to support reading the said packet "features". Many structs involved in the packet path from the socket to the ReceiveRTP method implement either PacketConn or NetConn. These interfaces however, cannot accommodate the needed ReadWithAttributes method. It is exactly the same problem for having a []byte as a packet, as it prevents further feature development. For the said reasons, Two interfaces were introduced, one for PacketConn and one for NetConn.
The names are not the best names, so any suggestions would be appreciated.