Skip to content

Replace KJ_DEFER with kj::defer#210

Closed
Sjors wants to merge 3 commits intobitcoin-core:masterfrom
Sjors:2025/08/kj-defer
Closed

Replace KJ_DEFER with kj::defer#210
Sjors wants to merge 3 commits intobitcoin-core:masterfrom
Sjors:2025/08/kj-defer

Conversation

@Sjors
Copy link
Copy Markdown
Member

@Sjors Sjors commented Sep 10, 2025 •
edited
Loading

My understanding is that this doesn't really matter much, e.g. tools like tidy and TSan won't work better. They might produce slightly more readable output.

It should however work for the all the versions of capnp that we support.

Based on #201

@ryanofsky
doc: describe ThreadContext struct and synchronization requirements
90d4a42
@ryanofsky
bug: fix ProxyClient<Thread> deadlock if disconnected as IPC call is …
7a8a99c 
…returning

This bug is currently causing mptest "disconnecting and blocking" test to
occasionally hang as reported by maflcko in
bitcoin/bitcoin#33244.

The bug was actually first reported by Sjors in
Sjors/bitcoin#90 (comment) and there are
more details about it in
bitcoin-core#189.

The bug is caused by the "disconnecting and blocking" test triggering a
disconnect right before a server IPC call returns. This results in a race
between the IPC server thread and the onDisconnect handler in the event loop thread both
trying to destroy the server's request_threads ProxyClient<Thread> object when
the IPC call is done.

There was a lack of synchronization in this case, fixed here by adding
loop->sync() various places. There were also lock order issues where
Waiter::m_mutex could be incorrectly locked before EventLoop::m_mutex resulting
in a deadlock.
@DrahtBot
Copy link
Copy Markdown

DrahtBot commented Sep 10, 2025 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #201 (bug: fix mptest hang, ProxyClient deadlock in disconnect handler by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

  • callback_thread and request_thread -> callback_threads and request_threads [the comment refers to the maps; the actual member names are plural "callback_threads" and "request_threads", so using singular form is inconsistent and may confuse readers]
  • before thread client is, -> before the thread client is destroyed, [phrase is missing the word "destroyed" (or similar) and the article "the", making the timing/meaning unclear]

drahtbot_id_5_m

@Sjors
Copy link
Copy Markdown
Member Author

Sjors commented Sep 10, 2025 •
edited
Loading

It should however work for the all the versions of capnp that we support.

I mainly opened this PR to test this assumption, since one of the jobs test early releases.

Feel free to close if it otherwise seems useless.

@Sjors
Replace KJ_DEFER with kj::defer
a5056a4 
Review hint:
git show --color-moved=dimmed-zebra --color-moved-ws=ignore-space-change
@DrahtBot DrahtBot mentioned this pull request Sep 10, 2025
Merged
@DrahtBot
Copy link
Copy Markdown

DrahtBot commented Sep 17, 2025

🐙 This pull request conflicts with the target branch and needs rebase.

@Sjors
Copy link
Copy Markdown
Member Author

Sjors commented Sep 17, 2025

CI passed, but closing this for now.

@Sjors Sjors closed this Sep 17, 2025
@Sjors Sjors mentioned this pull request Feb 19, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Needs rebase

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Sjors @DrahtBot @ryanofsky