Fix race condition in AsyncCurrentValueSubject
#40
Conversation
|
Hello! I encountered a similar problem. This fix seems to help me. Is there a plan to merge? |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29ad29a4ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let consumerId = self.state.withCriticalRegion { state -> Int in | ||
| state.ids += 1 | ||
| state.channels[state.ids] = asyncBufferedChannel | ||
| asyncBufferedChannel.send(state.current) | ||
| return state.ids |
There was a problem hiding this comment.
Calling asyncBufferedChannel.send(state.current) while inside state.withCriticalRegion can synchronously resume an awaiting consumer. If that consumer’s code touches the subject again (e.g., reads value or calls send), it will try to re-enter the same ManagedCriticalState that uses a non‑reentrant os_unfair_lock, leading to a deadlock. This only occurs when a consumer awaits the channel and then re-enters the subject on receipt, but that pattern is common in for‑await loops.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex What do you suggest that solves the initial issue while avoiding re-entrency ?
There was a problem hiding this comment.
@codex What do you suggest that solves the initial issue while avoiding re-entrency ?
There was a problem hiding this comment.
Codex couldn't complete this request. Try again later.
| let consumerId = self.state.withCriticalRegion { state -> Int in | ||
| state.ids += 1 | ||
| state.channels[state.ids] = asyncBufferedChannel | ||
| asyncBufferedChannel.send(state.current) | ||
| return state.ids |
There was a problem hiding this comment.
Calling asyncBufferedChannel.send(state.current) while inside state.withCriticalRegion can synchronously resume an awaiting consumer. If that consumer’s code touches the subject again (e.g., reads value or calls send), it will try to re-enter the same ManagedCriticalState that uses a non‑reentrant os_unfair_lock, leading to a deadlock. This only occurs when a consumer awaits the channel and then re-enters the subject on receipt, but that pattern is common in for‑await loops.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex What do you suggest that solves the initial issue while avoiding re-entrency ?
There was a problem hiding this comment.
To use Codex here, create an environment for this repo.
3 participants
Description
If a value is sent to the subject while
handleNewConsumer()is running, it is possible for the new consumer to miss this value. This is because thecurrentValueis read before the state is updated with the channel. This is addressed by setting the current value at the same point that the channel is added to the state.Checklist