fix multiple race conditions when calling Conn.Close()

datarhei / gosrt

Implementation of the SRT protocol in pure Go

MIT License

114 stars 17 forks source link

Hello, calling Conn.Close() currently results in multiple race conditions and in possible crashes due to the fact that some channels are closed before routines that make use of them are closed.

Closing a channel while writing to it causes a race condition, because writing to a channel is a read+write operation, and closing a channel and then writing to it causes a crash.

In Conn.Close() shutdown is set to true and close(c.writeQueue) is called, but it's still possible for another routine to write to c.writeQueue because this routine may have already passed the isShutdown() check:

if c.isShutdown() {
    return 0, io.EOF
}

// shutdown = true is called by another routine in parallel

// consequently, this can be called anyway and results either in a race condition or in a crash
select {
case c.writeQueue <- p:
}
...

The correct way to deal with this issue in an atomic way consists in using a context. In Conn.Close(), this context is canceled:

c.ctxCancel()

In routines, isShutdown() is removed and the select {} statement is improved in order to abort the read operation if the context is canceled:

select {
case <-c.ctx.Done():
case c.writeQueue <- p:
}

The same can be done for all read and write operations on channels.

isShutdown(), shutdown and shutdownMutex become useless and they can be removed.

The same context can be used to terminate all routines concurrently, therefore existing contexts are removed and all routines are linked to the same context.

Codecov Report

Patch coverage: 75.00% and project coverage change: -0.07% :warning:

Comparison is base (6f06385) 62.98% compared to head (7018d5a) 62.91%. Report is 4 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 #21 +/- ## ========================================== - Coverage 62.98% 62.91% -0.07% ========================================== Files 14 14 Lines 4530 4495 -35 ========================================== - Hits 2853 2828 -25 + Misses 1402 1399 -3 + Partials 275 268 -7 ``` | Flag | Coverage Δ | | |---|---|---| | unit-linux | `62.91% <75.00%> (-0.07%)` | :arrow_down: | 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. | [Files Changed](https://app.codecov.io/gh/datarhei/gosrt/pull/21?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datarhei) | Coverage Δ | | |---|---|---| | [connection.go](https://app.codecov.io/gh/datarhei/gosrt/pull/21?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datarhei#diff-Y29ubmVjdGlvbi5nbw==) | `49.37% <75.00%> (-0.48%)` | :arrow_down: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/datarhei/gosrt/pull/21/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=datarhei)

datarhei / gosrt

fix multiple race conditions when calling Conn.Close() #21

Codecov Report