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

Make actor.Stop respect the given exit reason #88

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

Conversation

sbergen
Copy link
Contributor

@sbergen sbergen commented Nov 28, 2024

Resolves #89

The actor would stop with Normal before this.

src/gleam/otp/actor.gleam Outdated Show resolved Hide resolved
@sbergen sbergen changed the title Make actor.Stop(process.Abnormal(...)) exit the process with the given reason Make actor.Stop respect the given exit reason Nov 28, 2024
@sbergen sbergen force-pushed the fix-actor-abnormal-exit branch from 2147e40 to a510de5 Compare November 28, 2024 20:18
@sbergen
Copy link
Contributor Author

sbergen commented Nov 29, 2024

I noticed afterwards, that there's some overlap between this and #86 Sorry for not spotting it earlier. I'll take a closer look at what was done there later.

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you. Couple questions inline

case reason {
Abnormal(reason) -> process.send_abnormal_exit(process.self(), reason)
Killed -> process.kill(process.self())
_ -> Nil
Copy link
Member

Choose a reason for hiding this comment

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

No catch all patterns ever please 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced it with Stop(_)👍

I also removed the dependency to gleam-lang/erlang#65 / gleam-lang/erlang#67, even though one of the tests is a bit wonky now 🤷

@@ -404,10 +409,10 @@ fn initialise_actor(
loop(self)
}

// The init failed. Exit with an error.
// The init failed. Send the reason back to the parent, but exit normally.
Copy link
Member

Choose a reason for hiding this comment

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

Why do we now exit normally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

failed_init_test would otherwise fail with the test process being killed. It's also what I'd assume to be correct based on the docs:

Failed(String)
The actor has failed to initialise. The actor shuts down and an error is returned to the parent process.

"shuts down" implies a clean exit to me.

The reason the test wasn't failing before is that exit_process didn't respect the reason given to it.

If we don't want a clean exit, then I think actor.Failed shouldn't exist, or it should at least have clearer documentation (similar reasoning to what I said here).

@sbergen sbergen force-pushed the fix-actor-abnormal-exit branch from a510de5 to c726c63 Compare December 9, 2024 20:05
@sbergen sbergen requested a review from lpil December 9, 2024 20:09
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.

actor.Stop always exists the process with Normal, instead of the given exit reason
2 participants