Fix a locking issue in the Rollback manager #6426
Conversation
briankassouf
dismissed stale reviews from ncabatoff and michelvocks
via
9b7fdf3
| return rsInflight | ||
| } | ||
|
|
||
| cancelCtx, cancelFunc := context.WithCancel(context.Background()) |
There was a problem hiding this comment.
Should this not derive the context using the ctx param and not context.Background()?
There was a problem hiding this comment.
I don't think it's necessary, doesn't need to be tied to any other external states
| // If we stopped due to shutdown, return. Otherwise another thread | ||
| // is holding the lock for us, continue on. | ||
| select { | ||
| case <-m.shutdownCh: |
There was a problem hiding this comment.
Will it be possible to read from this channel both in here and in the goroutine above?
There was a problem hiding this comment.
In the above goroutine, in case of a shutdown, stopCh will be closed by reading m.shutdownCh. We are trying to read from the same again here.
There was a problem hiding this comment.
You can read from a closed channel multiple times
There was a problem hiding this comment.
Remember that a receive from a closed channel returns immediately. So that's okay, we're not trying to consume anything, just use it as a signal.
|
@briankassouf This is already merged but I had 2 minor comments. Otherwise it looked good. |
No description provided.