Using Reopen() to flush built-up readBuffer#576
Conversation
| // Creates a new underlying UDP connection on the same port. | ||
| // The practical use of this function is to discard the assicuated OS-level read buffer of the old socket. | ||
| func (c *udpConn) Reopen() error { | ||
| lc := &net.ListenConfig{ |
There was a problem hiding this comment.
It is possible to enable SO_REUSEADDR here, instead of on creation, but would require an entire rework of these interface chains. Avoiding that for now.
| lc := &net.ListenConfig{ | ||
| Control: func(network, address string, c syscall.RawConn) error { | ||
| return c.Control(func(fd uintptr) { | ||
| syscall.SetsockoptInt(int(fd), syscall.SOL_SOCKET, syscall.SO_REUSEADDR, 1) |
There was a problem hiding this comment.
Ideally, anything that has to do with syscall should be in a separate file, under a build flag. This also implies that there should be a file that provides a fallback if these syscalls are not available.
So maybe we could solve it a bit more naively to avoid these portability issues? For example, we could just run the read loop on creation, but do not pass packets down until the port is explicitly enabled.
There was a problem hiding this comment.
we could just run the read loop on creation
That would have been ideal, and was my first go-to approach. But sure to how things are structured right now, we can one of the following:
- This PR or a variation (no reopen, just straight up close and immediately open on the same port).
- Start a "custom" rtp loop - just read & discard function until told to stop. Not reusing the
Sessiontype. - Start a dummy
Sessiontype, have it run the normal, then, when we receive the answer SDP (and thus have*MediaConf), we can determine whether itssrtp.NewSession(p.log, p.port, c.Crypto)orrtp.NewSession(p.log, p.port)and drop the old session. - Ideally, I'd like to be able to start the normal session without knowing the selected codecs/crypto just yet, but that requires quite a lot of changes that I'm not sure need to be taken on today.
All these aren't great to some extent. I think option 2 is easy enough as well, but I'd like to avoid 3.
Do you have any specific preferences on this?
There was a problem hiding this comment.
There was a problem hiding this comment.
@alexlivekit I'm probably missing something important that makes it harder to do so, but what if we just run rtpLoop in MediaPort on port creation? This will automatically run rtpReadLoop, which will start consuming packets. However, until MediaPort.hnd is not set it will drop them immediately. So we can make it only enable that handler after we SetConfig. That should work, right?
There was a problem hiding this comment.
You need the session for that, and you need the config for the session. So:
SDP answer -> config -> sessoin -> rtploop.
To change this I'd need to rewire quite a bit, hence opting not to.
There was a problem hiding this comment.
Ah, yeah, you are right, I missed that part. Lets go with your proposed option 2. But please push it here first, there are some comments there. For example, rename stop to discard, and wait for the discard loop to stop before starting a real read loop.
There was a problem hiding this comment.
Updated.
The main problem with this approach imo is if we do DTLS with carriers, this may eat up server hello messages, delaying media establishment until the call is actually answered.
Edit: Never mind, this doesn't happen, as we do not emit the client hello until after setting the SDP answer.
There was a problem hiding this comment.
That shouldn't be a problem, since we don't use DTLS, we use SRTP only. It doesn't have hello messages, I think.
alexlivekit
force-pushed
the
alexmigolin/tel-322-ignore-early-packets-for-outbound-calls
branch
from
6edcc87 to
068b4d1
Compare
alexlivekit
deleted the
alexmigolin/tel-322-ignore-early-packets-for-outbound-calls
branch
2 participants
From initial PR, timeline:
This PR is here to address that issue.