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

Add Supervisor shutdown options #65

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

Conversation

nerdyworm
Copy link

@nerdyworm nerdyworm commented Jun 6, 2024

The original issue that I was facing was that supervisors never actually killed their child processes, thus leading to a memory/process leak when my system was running for about a week.

This pull request implements the two strategies that erlang's supervisor employees, timeout and brutal kill.

The root cause of why actors never exit with erlang:exit(pid, normal) is still unknown.

@nerdyworm
Copy link
Author

Update: I discovered that using erlang:exit(Pid, normal) from an external process does not stop the process.

I've updated my PR to use erlang:exit(Pid, shutdown). This successfully stops the actors and everything looks perfect.

I've also discovered if your actor traps exits, then you'll have to match on the following in your actor to shut it down, otherwise, it will timeout -> brutal kill.

 Down(ExitMessage(_supervisor_pid, Abnormal("Shutdown"))) -> {
      actor.Stop(Abnormal("shutdown"))
  }

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.

Oh wonderful! Thank you for digging into this! 💜

Looks great, though I've left some small notes inline.

ChildSpec(..child, shutdown: Timeout(timeout))
}

pub fn shutdown_brutal_kill(
Copy link
Member

Choose a reason for hiding this comment

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

What's this function for? It is undocumented.

ChildSpec(start: child.start, returning: updater, shutdown: child.shutdown)
}

/// Change the shutdown timeout
Copy link
Member

Choose a reason for hiding this comment

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

Could you add more documentation here please 🙏

let result =
process.new_selector()
|> process.selecting_anything(fn(a) { a })
|> process.select(0)
Copy link
Member

Choose a reason for hiding this comment

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

This drops the first message in the inbox, but this may not be the exit message. It needs to drop specifically the expected message here.

// Ensure that the original processes are dead
should.be_false(process.is_alive(pid1))
should.be_false(process.is_alive(pid2))
should.be_false(process.is_alive(pid3))
Copy link
Member

Choose a reason for hiding this comment

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

Could you move these to a new test please 🙏

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