Skip to content

Fix deadlock on request timeout#389

Merged
rebello95 merged 1 commit into
connectrpc:mainfrom
headlinersapp:fix-timeout-deadlock
Apr 2, 2026
Merged

Fix deadlock on request timeout#389
rebello95 merged 1 commit into
connectrpc:mainfrom
headlinersapp:fix-timeout-deadlock

Conversation

@MartinJrP

@MartinJrP MartinJrP commented Mar 6, 2026

Copy link
Copy Markdown
Contributor

Overview

When creating a ProtocolClient with a timeout TimeoutTimer deadlocks when the deadline is reached. If onTimeout calls TimeoutTimer.cancel the thread deadlocks because cancel blocks execution of the work item.

ProtocolClient doesn't trigger this directly -- rather when TimeoutTimer gets deallocated it calls cancel.

Fix

DispatchWorkItem is thread safe so there's no need to call it on the class' private queue.

Screenshot 2026-03-06 at 5 32 17 PM Screenshot 2026-03-06 at 5 31 12 PM

@MartinJrP MartinJrP force-pushed the fix-timeout-deadlock branch 2 times, most recently from bc31fa7 to cfcb401 Compare March 6, 2026 22:59
@MartinJrP MartinJrP changed the title fix: Deadlock on request timeout Fix deadlock on request timeout Mar 7, 2026
@rebello95

Copy link
Copy Markdown
Collaborator

Thank you for the submission @MartinJrP! It doesn't look like the testStreamRequestTimesOut test is passing on CI, is it passing for you locally?

@MartinJrP MartinJrP force-pushed the fix-timeout-deadlock branch from cfcb401 to 40b86c6 Compare March 31, 2026 21:32
Signed-off-by: Martin Jr Powlette <hi@martinjrpowlette.com>
@MartinJrP MartinJrP force-pushed the fix-timeout-deadlock branch from 40b86c6 to 5c5355e Compare March 31, 2026 23:00
@MartinJrP

Copy link
Copy Markdown
Contributor Author

Sorry about that @rebello95 and thanks for your patience! Should be good to go now -- found another deadlock

@rebello95 rebello95 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

thank you!

@rebello95 rebello95 merged commit d9eda55 into connectrpc:main Apr 2, 2026
23 of 24 checks passed
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.

2 participants