Skip to content

fix: prevent deadlock on concurrent push and disconnect (#24215) (CP: 25.1)#24227

Merged
vaadin-bot merged 1 commit into25.1from
cherry-pick-24215-to-25.1-1777550606321
Apr 30, 2026
Merged

fix: prevent deadlock on concurrent push and disconnect (#24215) (CP: 25.1)#24227
vaadin-bot merged 1 commit into25.1from
cherry-pick-24215-to-25.1-1777550606321

Conversation

@vaadin-bot
Copy link
Copy Markdown
Collaborator

This PR cherry-picks changes from the original PR #24215 to branch 25.1.

Original PR description

The previous AtomicBoolean guard in AtmospherePushConnection closed only the disconnect-vs-disconnect race. A push thread that reads disconnecting as false before a concurrent disconnect() flips it can still proceed into synchronized(lock) behind the disconnect thread, which is itself blocked inside resource.close() waiting for the servlet container's HTTP session lock held by the push thread — a two-lock cycle.

Move resource.close() out of the monitor: inside synchronized(lock) capture the resource into a local and call connectionLost() to transition the state, then release the monitor before invoking close() on the stashed reference. Add a matching re-check of isConnected() at the top of the synchronized block in push() so a push that waited for the monitor observes the late disconnect and defers via PUSH_PENDING/RESPONSE_PENDING instead of NPEing on the cleared resource. The disconnecting flag stays set until close() returns so subsequent pushes take the fast path and no new disconnect() re-enters while close() is still in flight.

Related-to #24192

The previous `AtomicBoolean` guard in `AtmospherePushConnection` closed
only the disconnect-vs-disconnect race. A push thread that reads
`disconnecting` as false before a concurrent `disconnect()` flips it can
still proceed into `synchronized(lock)` behind the disconnect thread,
which is itself blocked inside `resource.close()` waiting for the
servlet container's HTTP session lock held by the push thread — a
two-lock cycle.

Move `resource.close()` out of the monitor: inside `synchronized(lock)`
capture the resource into a local and call `connectionLost()` to
transition the state, then release the monitor before invoking `close()`
on the stashed reference. Add a matching re-check of `isConnected()` at
the top of the `synchronized` block in `push()` so a push that waited
for the monitor observes the late disconnect and defers via
`PUSH_PENDING`/`RESPONSE_PENDING` instead of NPEing on the cleared
resource. The `disconnecting` flag stays set until `close()` returns so
subsequent pushes take the fast path and no new `disconnect()` re-enters
while `close()` is still in flight.

Related-to #24192
@vaadin-bot
Copy link
Copy Markdown
Collaborator Author

This PR is eligible for auto-merging policy, so it has been approved automatically. If there are pending conditions, auto merge (with 'squash' method) has been enabled for this PR [Message is sent from bot]

@vaadin-bot vaadin-bot enabled auto-merge (squash) April 30, 2026 12:12
@vaadin-bot
Copy link
Copy Markdown
Collaborator Author

This PR is eligible for auto-merging policy, so it has been approved automatically. If there are pending conditions, auto merge (with 'squash' method) has been enabled for this PR[Message is sent from bot]

@sonarqubecloud
Copy link
Copy Markdown

@github-actions
Copy link
Copy Markdown

Test Results

 1 387 files  ±0   1 387 suites  ±0   1h 19m 9s ⏱️ + 2m 24s
10 015 tests +1   9 945 ✅ +1  70 💤 ±0  0 ❌ ±0 
10 488 runs  +1  10 409 ✅ +1  79 💤 ±0  0 ❌ ±0 

Results for commit b805120. ± Comparison against base commit 1c8e929.

@vaadin-bot vaadin-bot merged commit e3b425c into 25.1 Apr 30, 2026
29 checks passed
@vaadin-bot vaadin-bot deleted the cherry-pick-24215-to-25.1-1777550606321 branch April 30, 2026 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants