Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

First implementation bits (long-running PR) #7

Merged
merged 8 commits into from Mar 25, 2023
Merged

First implementation bits (long-running PR) #7

merged 8 commits into from Mar 25, 2023

Conversation

NicolasT
Copy link
Owner

@NicolasT NicolasT commented Mar 19, 2023

Some code dropping. Not necessarily the way we want things to be.

  • Exception handling, and propagation to linked and monitoring processes.
  • CQueue, taken from distributed-process, is quite complicated. We may want to make this pure STM. (CQueue: rework implementation #11)
  • Rework CQueues "timeout" support using STM's registerDelay. (CQueue: use registerDelay #12)
  • Currently, there's MonadProcess which exposes some somewhat-high-level actions. However, these can be reimplemented by making MonadProcess more low-level. Basically, we'll want a way to retrieve the current ProcessContext, and some other Maybe ProcessContext based on a ProcessId (in STM). With those in place (and maybe some others), we can implement sendWithOptions, spawnWithOptions etc. outside of the class.
  • Review/validate spawnImpl

Fixes: #25

@NicolasT
Copy link
Owner Author

Anyone interested to review this, I'd appreciate a review of the spawnImpl function in Process.hs. It's not unlikely there's a bug in there right now, in case an asynchronous exception reaches the monitor thread of a Process thread.

@NicolasT NicolasT force-pushed the hack branch 9 times, most recently from 06e5613 to a8c6f5c Compare March 22, 2023 17:23
@NicolasT NicolasT marked this pull request as ready for review March 22, 2023 17:37
@NicolasT NicolasT added the help wanted Extra attention is needed label Mar 22, 2023
While working on some other code, it appeared there's a bug with
`spawnLink`. However, after adding this test-case, it doesn't appear to
be the case (so further debugging in the other branch is required).

Keeping the test-case around doesn't harm, though.
@NicolasT NicolasT marked this pull request as ready for review March 24, 2023 23:12
@NicolasT NicolasT linked an issue Mar 24, 2023 that may be closed by this pull request
When a process is both linked to as well as monitoring some other
process, we want to make sure any exception is propagated through the
link before a `Down` message can be observed by said process after which
it could, e.g., exit successfully, in case its monitor thread did not
(yet) observe the signal to inject.

This can be achieved by ensuring there are no pending signals at the
end of the `receiveWithOptions` implementation, before delivering any
matched message to the caller.

This patch includes a test-case which, before the fix, exposes the issue
(though this is a bug in concurrency, so it may succeed once in a
while). With the fix in place it should now succeed consistently.

Fixes: #25
See: #25
@NicolasT
Copy link
Owner Author

@TristanCacqueray With #21 merged here, any chance you could approve this one so it can go in main? I'm somewhat-confident things will mostly work, though as mentioned, spawnImpl could have some subtle issues, which we'll need to flesh out over time 😃 (#17 could help in this area as well).

This doesn't imply the current design/implementation is "right", happy to look into various improvements/changes/... to evolve the code.

@NicolasT NicolasT enabled auto-merge March 25, 2023 16:06
Copy link
Collaborator

@TristanCacqueray TristanCacqueray left a comment

Choose a reason for hiding this comment

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

I haven't reviewed it all, but that looks good to me for merging.

@NicolasT NicolasT merged commit a9fb3ab into main Mar 25, 2023
12 checks passed
@NicolasT NicolasT deleted the hack branch March 25, 2023 16:06
@NicolasT NicolasT linked an issue Mar 25, 2023 that may be closed by this pull request
@TristanCacqueray
Copy link
Collaborator

Excellent, thank you, I'll give HEAD a try in a test project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deliver signals before/when receiving messages Add spawnOn and spawnBounded, or similar
2 participants