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

Implementation of the exit_process for Actor #86

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

bighelmet7
Copy link
Contributor

@bighelmet7 bighelmet7 commented Nov 22, 2024

Context

In my journey of learning how the OTP works, when I tried to send an exit signal to an Actor I noticed that the exit_process wasn't fully implemented. So, I tried to give it a try and implement it based on:

Apologies in advance if this isn't the correct way of implementing the Shutdown protocol. I will be more than happy to amend the PR with a more correct Shutdown protocol based on the received feedback.

Changes

  • linting on the otp/actor.gleam (I can move this to a separate PR, if necessary).
  • exit_process now supports Normal, Killed and Abnormal exit reasons.
  • fix the broken failed_init_test: this test was blocking the whole actor test from running due to an unhandled crash.
  Compiling gleam_otp
   Compiled in 0.29s
    Running gleam_otp_test.main
....
Pending:
  undefined
    %% Related process exited with reason: {abnormal,<<"not enough wiggles">>}
  • tests for the exit_process implementation: this covers the Normal, Killed and Abnormal reasons.

@sbergen
Copy link
Contributor

sbergen commented Nov 30, 2024

Hi, @bighelmet7! I ran into the same issue recently, but didn't spot your PR when looking for relevant PRs, as the title didn't include any references to the public functionality that was misbehaving (which I would say is actor.Stop). exit_process is private function in the module, so when I was first looking for relevant issues, I didn't recognize this as related.

I haven't been doing much open source stuff on GitHub recently (other than my own projects), so I also didn't think of opening an issue in addition to a PR, to bring more visibility for what is actually being fixed.

@lpil it might be a good idea to add CONTRIBUTING.md files similar to the one in the main Gleam repo to encourage opening descriptive issues in addition to PRs, to avoid issues like this in the future.

As for the content of the PR itself, comparing to my PR, the main difference in the implementation seems to be that here we're still exiting the process with an abnormal reason after a failed init. In my opinion, this should not happen, as the error is already included in the return value of start_spec, so a user of the function should not need to handle any abnormal exit signals in addition to this.

Also, I'm using selecting_trapped_exits in tests to avoid having to start the actor in a separate unlinked process. However, when doing this, I ran into this issue, which complicated things.

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