be more specific with `die` reasons in Supervisor

haskell-distributed / distributed-process-platform

DEPRECATED (Cloud Haskell Platform) in favor of distributed-process-extras, distributed-process-async, distributed-process-client-server, distributed-process-registry, distributed-process-supervisor, distributed-process-task and distributed-process-execution

http://haskell-distributed.github.com

BSD 3-Clause "New" or "Revised" License

47 stars 18 forks source link

be more specific with `die` reasons in Supervisor #87

Closed tavisrudd closed 10 years ago

tavisrudd commented 10 years ago

Several calls to die were using opaque reasons that were unhelpful to developers during debugging.

hyperthunk commented 10 years ago

Hmn, I wonder if we should die with an object instead of just a string here. What do you think? Do we ever want to actually catchExit and handle these?

tavisrudd commented 10 years ago

I'm not sure if it's worth it in this case. One is an internal error. The other is a failure to resolve the return value from the proc passed to toChildStart. That happens inside the starterProcess so there's no chance to catch it.

I'm not sure the die is sufficient here for the same reason. It would kill the starterProcess, which via a monitor eventually propagates out to the unfinished Left case in doRestartChild. The related childSpec would be removed. I think we need to at least log the failure cases in doRestartChild. Should the starterProcess be restarted?

hyperthunk commented 10 years ago

I think you're right - it's not worth changing.

I think we need to at least log the failure cases in doRestartChild.

Yes I agree with that. Notice or error level I think, probably error IMO.

Should the starterProcess be restarted?

I don't see how that's possible given the nature of the StarterProcess constructor - it just takes a pid and knows nothing about how the restarter process is/was implemented. I think we've simply got to treat that as a failure. Questions arise though:

under what circumstances will this remove the child spec?
depending on the restart type and policies in place, should this bring down the rest of the tree?
do both those aspects actually work in code right now, or have we found a breakage/bug here?

tavisrudd commented 10 years ago

I think it should bring down the tree in the same cases where the child would be restarted: all but Temporary. Let's merge these die messages and I'll work on this in a separate PR. It might be best to include some extra error reporting mechanism for this case in your suggestion of spawnChildStarter on PR #77.

hyperthunk commented 10 years ago

Had to do this by hand due to merge conflicts. Thanks @tavisrudd - I'll get to the rest of the patches now.