Skip to content

Fix destroy handler race #141

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Jan 20, 2025
Merged

Conversation

echistyakov
Copy link
Contributor

Fixes a race in destroyHandler w.r.t. dc.messages sync.Map

Motivation:

This PR is meant to fix a contributing factor to the following race PR: jjeffcaii/reactor-go#45

In the current implementation of destroyHandler - it's possible to call stopWithError on a callback that has already been unregistered. This can affect a completely unrelated Duplex object (because global pools are used for processor objects - and an already "disposed" processor can receive an error).

The race (for example) can occur between the following pieces of code:

func (dc *DuplexConnection) destroyHandler(err error) {
// TODO: optimize callback map
var callbacks []callback
dc.messages.Range(func(_, value interface{}) bool {
callbacks = append(callbacks, value.(callback))
return true
})
for _, next := range callbacks {
next.stopWithError(err)
}
}

onFinally := func(s reactor.SignalType, d reactor.Disposable) {
common.TryRelease(handler.cache)
if s == reactor.SignalTypeCancel {
dc.sendFrame(framing.NewWriteableCancelFrame(sid))
}
// Unregister handler w/sink (processor).
dc.unregister(sid)
// Dispose sink (processor).
d.Dispose()
}

They both access dc.messages. destroyHandler - for reading (iteration), onFinally - for writing (inside unregister method).

In the existing version of destroyHandler - by the time iteration (Range()) over dc.messages is complete - some of the messages may have already been unregistered and removed from the map. This is where the race comes from.

As per documentation: https://pkg.go.dev/sync#Map.Range

Range does not block other methods on the receiver

-> Meaning that it's totally safe to invoke every callback from inside Range - no need to store callbacks in a slice beforehand.

Modifications:

Perform callback stopWithError invocations directly during Range iteration, without creating a slice of callbacks beforehand.

Result:

The issue described in jjeffcaii/reactor-go#45 goes away.

Copy link
Member

@jjeffcaii jjeffcaii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jjeffcaii jjeffcaii merged commit 28d7b65 into rsocket:master Jan 20, 2025
2 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants