fix: avoid data race in session signal channel register/deregister #5
Conversation
0786b47 to
f6b35a4
Compare
f6b35a4 to
9fde237
Compare
| } | ||
| go func() { | ||
| defer sess.sigMu.Unlock() |
There was a problem hiding this comment.
this is unorthodox enough to warrant a comment; it looked like a bug to me at first.
There was a problem hiding this comment.
what I meant is the fact that you're calling
defer sess.sigMu.Unlock()
without the corresponding Lock() in the same function looks very strange to me! We often talk about a function "holding" the lock while it runs, but here you "transfer" the lock from the Signals() function to the child goroutine so the parent can return.
There was a problem hiding this comment.
Yes, I understood that's what you meant. Did you feel the motivation for it was lacking?
There was a problem hiding this comment.
As an aside, this is explicitly documented in https://pkg.go.dev/sync#Mutex.Unlock
A locked Mutex is not associated with a particular goroutine. It is allowed for one goroutine to lock a Mutex and then arrange for another goroutine to unlock it.
Not that I'm making any claims about this being standard in any way... I was simply trying to retrofit an edge-cases covered fix into the existing code without too many changes.
Interestingly, there's been a proposal to disallow this, but it didn't gain traction: golang/go#9201
There was a problem hiding this comment.
It was actually just about the mechanics --- like
// here we're relying on the fact that the mutex was locked in the outer `Signal()`
// function, so this goroutine/function just has to unlock the mutex when it is done
We recently added signal support to
agentssh, however, I noticed the following goleak flake today:https://github.com/coder/coder/actions/runs/7003589751/job/19049631559?pr=10883
This made me take a closer look at the implementation and saw that it was racy. This PR fixes the race.