datarhei / gosrt

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

fix multiple race conditions in connection tests #20

Closed aler9 closed 11 months ago

aler9 commented 11 months ago

Hello, in tests of connection there are multiple data race issues linked to the fact that a bytes.Buffer is written and read by two routines in parallel.

Current code is more or less this one:

readerWg := sync.WaitGroup{}
readerWg.Add(1)

dataReader1 := bytes.Buffer{}

go func() {
    conn, err := Dial("srt", "127.0.0.1:6003", config)
    if !assert.NoError(t, err) {
        panic(err.Error())
    }

    buffer := make([]byte, 2048)

    readerWg.Done()

    for {
        n, err := conn.Read(buffer)
        if n != 0 {
            dataReader1.Write(buffer[:n])
        }
    }
}()

readerWg.Wait()

reader1 := dataReader1.String()

dataReader1.Write() is called concurrently with dataReader1.String(), resulting in the data race.

This patch fixes the issue by replacing the WaitGroup with two channels that allow to wait on two distinct steps, whether Dial() has returned and whether the routine has returned:

readerConnected := make(chan struct{})
readerDone := make(chan struct{})

dataReader1 := bytes.Buffer{}

go func() {
    defer close(readerDone)

    conn, err := Dial("srt", "127.0.0.1:6003", config)
    if !assert.NoError(t, err) {
        panic(err.Error())
    }

    buffer := make([]byte, 2048)

    <-readerConnected

    for {
        n, err := conn.Read(buffer)
        if n != 0 {
            dataReader1.Write(buffer[:n])
        }
    }
}()

<-readerConnected

...

<-readerDone

reader1 := dataReader1.String()

This patch also removes serverWg.Done() before ListenAndServe() since it's totally useless since ListenAndServe() doesn't return until server is shutted down.

codecov-commenter commented 11 months ago

Codecov Report

Patch and project coverage have no change.

Comparison is base (6f06385) 62.98% compared to head (bfc93d7) 62.98%. Report is 6 commits behind head on main.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #20 +/- ## ======================================= Coverage 62.98% 62.98% ======================================= Files 14 14 Lines 4530 4530 ======================================= Hits 2853 2853 + Misses 1402 1401 -1 - Partials 275 276 +1 ``` | Flag | Coverage Δ | | |---|---|---| | unit-linux | `62.98% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datarhei#carryforward-flags-in-the-pull-request-comment) to find out more. [see 3 files with indirect coverage changes](https://app.codecov.io/gh/datarhei/gosrt/pull/20/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datarhei)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.