-
Notifications
You must be signed in to change notification settings - Fork 3k
Allow monitor options in proc_lib:start_monitor/5
#9804
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
base: master
Are you sure you want to change the base?
Allow monitor options in proc_lib:start_monitor/5
#9804
Conversation
CT Test Results 4 files 221 suites 1h 55m 14s ⏱️ Results for commit d11e342. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts// Erlang/OTP Github Action Bot |
| is_atom(F), | ||
| is_list(A) -> | ||
| ?VERIFY_NO_MONITOR_OPT(M, F, A, Timeout, SpawnOpts), | ||
| SpawnOpts1 = ensure_spawn_option(monitor, SpawnOpts), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW using [monitor, monitor] or [monitor, {monitor, Opts}] is just fine in spawn_opt so it might be fine to just add this option unconditionally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but which one of conflicting options (eg monitor vs {monitor, Opts}) wins is undocumented (currently, it is the last one in the list). So if we want to play strictly by the book ("what's not documented may change, at any time, without notice"), we need to check.
Co-authored-by: Jan Uhlig <[email protected]>
6d9d85b to
bd983db
Compare
a1761e9 to
d11e342
Compare
|
The type |
|
Btw @rickard-green @jhogberg this PR has been lying around for quite a while now, since May. Is there any interest in it? |
I don't think supervisors need to handle this. It is the responsibility of the child start function to return something that the supervisor can handle. That said, the child start function is executed in the context of the supervisor process, so if it sets up a monitor there one way or other, all this leads to is that the supervisor receives an unexpected message when the child exits. About allowing |
Currently, the spawn options
monitorand{monitor, MonitorOpts}are disallowed in theproc_lib:start*functions. This goes back all the way to R11, a ticket OTP-6345, and this post on the old Erlang ML.For
proc_lib:start_monitor, it makes no sense to forbid monitor options. Or at least, we don't see any benefit in doing so. On the contrary, it prevents "customizing" the monitor (eg custom tags, usage of the newpriorityoption, etc). This PR removes this restriction.For
proc_lib:startandproc_lib:start_link, it would be possible to remove the restriction also, that is, without restoring the buggy behavior that seems to have given raise to OTP-6345.However, in the presence of a
monitoror{monitor, MonitorOpts}spawn option, the return value would change fromRetto{Ret, Mon}, with repercussions viageninto thestart/start_linkfunctions of thegen_*behaviors.This may be ok, and require mostly work on the specs and documentation in the
gen_*behaviors, aside from some adaptions ingen. The bigger question IMO is if and how supervisors should cope with this?This PR also does a few more things than what is listed above, which are related to the new
{link, LinkOpts}spawn option that appeared with the new priority messages. Those are mostly just documentation changes.