Skip to content

Comments

Fix a jetStream / jsAccount lock ordering violation#7710

Merged
neilalexander merged 1 commit intomainfrom
js-jsa-lock-inversion
Jan 9, 2026
Merged

Fix a jetStream / jsAccount lock ordering violation#7710
neilalexander merged 1 commit intomainfrom
js-jsa-lock-inversion

Conversation

@sciascid
Copy link
Contributor

@sciascid sciascid commented Jan 9, 2026

Method addStreamWithAssignment could cause a deadlock when acquiring the jetStream lock while holding the jsAccount lock, a lock ordering violation.
This would cause a deadlock when attempting to shutdown and a a stream was created concurrrently. Easiliy reproduced when running BenchmarkJetStreamCreate benchmark.
The jetstream lock was acquired to protect the following assignment:

        mset.create = sa.Created

It appears unnecessary to lock jetstream for that, so the fix removes the locking.

Signed-off-by: Daniele Sciascia daniele@nats.io

@sciascid sciascid requested a review from a team as a code owner January 9, 2026 10:21
// Add created timestamp used for the store, must match that of the stream assignment if it exists.
if sa != nil {
js.mu.RLock()
mset.created = sa.Created
Copy link
Member

Choose a reason for hiding this comment

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

I assume this is safe because sa.Created is a special-case field whose value never changes, if so it might be worth adding a comment to that effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the field appears to be immutable.
Running BenchmarkJetStreamCreate under -race shows no problems.
(BenchmakrJetStreamCreate would reliably fall into the deadlock execution)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment in the code to clarify that.

Method `addStreamWithAssignment` could cause a deadlock when
acquiring the jetStream lock while holding the jsAccount lock,
a lock ordering violation.
This would cause a deadlock when attempting to shutdown and a
a stream was created concurrrently. Easiliy reproduced when
running `BenchmarkJetStreamCreate` benchmark.
The jetstream lock was acquired to protect the following
assignment:

```
        mset.create = sa.Created
```
It appears unnecessary to lock jetstream for that, so the fix
removes the locking.

Signed-off-by: Daniele Sciascia <daniele@nats.io>
@sciascid sciascid force-pushed the js-jsa-lock-inversion branch from 4bddd3c to a51d73f Compare January 9, 2026 12:02
Copy link
Member

@MauriceVanVeen MauriceVanVeen left a comment

Choose a reason for hiding this comment

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

LGTM

@neilalexander neilalexander merged commit 6e7ad36 into main Jan 9, 2026
48 checks passed
@neilalexander neilalexander deleted the js-jsa-lock-inversion branch January 9, 2026 12:35
neilalexander added a commit that referenced this pull request Jan 15, 2026
Includes the following:

- #7704
- #7708
- #7710
- #7709
- #7712
- #7716
- #7720
- #7721
- #7723
- #7728

Signed-off-by: Neil Twigg <neil@nats.io>
neilalexander added a commit that referenced this pull request Jan 15, 2026
Includes the following:

- #7704
- #7708
- #7710
- #7711
- #7709
- #7712
- #7716
- #7720
- #7721
- #7723
- #7728

Signed-off-by: Neil Twigg <neil@nats.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants