datarhei / gosrt

Implementation of the SRT protocol in pure Go
https://datarhei.com
MIT License
114 stars 17 forks source link

Allow processing connection requests in parallel #62

Closed aler9 closed 3 months ago

aler9 commented 3 months ago

Fixes #59 Fixes https://github.com/bluenviron/mediamtx/issues/3382

Currently, connection requests are processed in series, in the same loop, through a callback that is passed as argument to Accept():

for {
   conn, mode, err := ln.Accept(func(req srt.ConnRequest) ConnType {
        // process request
        return srt.ACCEPTED
    })
    if err != nil {
        break
    }

    // conn is available
    conn.Close()
}

When the connection request is subjected to lengthy checks, the next request cannot be processed until the first one is done. This causes QoS issues on client-side (i.e. clients cannot connect) and a waste of resources on server-side (since computational power cannot be fully used).

This patch provides 3 additional methods called Listener.Accept2(), ConnRequest.Accept() and ConnRequest.Reject() that allow to solve the issue, in a way that is as similar as possible to the one of standard libraries:

for {
   connReq, err := ln.Accept2()
    if err != nil {
        break
    }

    go func() {
        // asynchronous logic
        if (keep) {
            conn, err := connReq.Accept()
            if err != nil {
                return
            }

            // conn is available
            conn.Close()
        } else {
            connReq.Reject(srt.REJ_PEER)
        }
    }()
}

This patch is fully backward compatible with the existing Accept() method and logic.

ioppermann commented 3 months ago

@aler9 Thanks a lot for this patch. I merged the PR.

There's still something to look into. Now it is possible to accept/reject a connection several times with ConnRequest.Accept() and ConnRequest.Reject() by accident. This should either return an error, the already accepted connection, or be a noop. Otherwise it will send the respective packets unnecessarily on the wire and mess with the previously accepted connection.

aler9 commented 2 months ago

@ioppermann thanks for the feedback and for maintaining the library.

The question about calling ConnRequest.Accept() or ConnRequest.Reject() twice is a little bit subjective, personally in my libraries i don't support calling Close() methods twice, but in the Golang standard library this is allowed. In order to implement it i think it's enough to wrap Accept and Reject inside a sync.Once:

struct connRequest {
    // existing fields
    once sync.Once
}

func (req *connRequest) Reject(reason RejectionReason) {
   req.once.Do(func() {
        // existing method
    })
}
ioppermann commented 2 months ago

Both functions will send some data on the wire that already has been sent. I prefer to prevent this if it can be done with little effort and it is nicer for the user of the library. Otherwise it must be clearly documented, that it should never be called more than once.

I solved it such that first req.ln.connReqs[req.socketId] gets checked if this request has already been processed (i.e. accepted or rejected). If yes, the entry for req.socketId will not be found and nothing will be sent to the wire. ConnRequest.Reject() will just do nothing and ConnRequest.Accept() will return an error.