ssh: use new supervisor:stop/1 function for stopping system

[u3s]

• see https://github.com/erlang/otp/pull/9209/files

[github-actions]

CT Test Results

    2 files     29 suites   19m 19s ⏱️
  481 tests   474 ✅  6 💤 1 ❌
1 686 runs  1 659 ✅ 26 💤 1 ❌

For more details on these failures, see this check.

Results for commit 53e1f07.

♻️ 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

• Complete CT logs (Download Logs)
• HTML Documentation (Download HTML Docs)
• No Windows Installer found

// Erlang/OTP Github Action Bot

[IngelaAndin]

Or maybe we think it is the supervisor that should catch the exception from gen_server:stop. We have time to fix that if we think so. Lets think about it.

[IngelaAndin]

Although I dislike the behavior of exiting the calling process if the process was already terminated (and the user of the supervisor should not need to know how it is implemented), this behavior has existed a long time for gen_processes and someone made an argument that it is the only way for some one that may consider this an error to detect it.

Also the supervisor PR actually both test and document this behavior, I did not really react to it as much as I was mainly concerned with other aspects of the PR and writing this off as gen_server behavior without fully understanding the consequences until ssh made a mistake in its test suite. I guess this would mostly happen by accident like a "double free in a c-program". Possibly also happen as a race if the process is terminated as due to a significant child terminating.

Also the gen_call timoute will exit the calling process which is less of a problem now that you do not get late replies from the server to a client catching that. But there could still be reasons to rather implement a server side timeout.

So maybe we should just live with it and you go ahead with this PR. @Maria-12648430 any thoughts?

[u3s]

• I updated the PR, so supervisor:stop exit behavior propagates to ssh API as well.
• I mentioned it in the docs
• this is a potentially incompatible change as previously when daemon process wasn't found function could return error tuple (this was missing in function spec) and with update it will exit the calling process

[Maria-12648430]

Although I dislike the behavior of exiting the calling process if the process was already terminated (and the user of the supervisor should not need to know how it is implemented), this behavior has existed a long time for gen_processes and someone made an argument that it is the only way for some one that may consider this an error to detect it.

I think you mean this comment by @RaimoNiskanen, and following?

I'm not sure if exiting is the most appropriate thing to do in such a case or whether {error, noproc} would be better, but then again, it may indeed hint at design flaws or bugs. In any case, it shouldn't be glossed over and silently ignored. Especially with supervisors, they are by and large part of a static structure, and stopping parts of that should be done with care. That is, if proper care is taken, double-stops shouldn't happen.

If you really don't care, or want to accept double-stops, it is possible to do so by try ... catch ...-ing the supervisor:stop call. The other way, ie making glossed-over things noticeable, is much harder.

Also the supervisor PR actually both test and document this behavior, I did not really react to it as much as I was mainly concerned with other aspects of the PR and writing this off as gen_server behavior without fully understanding the consequences until ssh made a mistake in its test suite. I guess this would mostly happen by accident like a "double free in a c-program".

I find it hard to imagine anything else than an accident, TBH.

Possibly also happen as a race if the process is terminated as due to a significant child terminating.

Which is also something which might be worth investigating. If it gets to be noticed.

So maybe we should just live with it and you go ahead with this PR. @Maria-12648430 any thoughts?

Hm, my tl;dr would be that, if you are willing to accept multiple stops in this specific case, just wrap the supervisor:stop call in a try ... catch .... But going by intuition, I'd rather not, and let the exit get to the surface.

[IngelaAndin]

@Maria-12648430 yes I was already inclined to think it was best to leave it as is, your thoughts I think helps even more to
feel it is the reasonable thing to do under the circumstances, and that exiting the user process most probably will be a bug and this should not disturb good code :) So lets be consistent with what we already have.

[u3s]

thanks for comments. I wasn't really after hiding that behavior, but was misguided with ssh:stop_daemon suggesting it always returns ok. this contract wasn't met as function could return error tuple in this case.

with supervisor:stop used it will exit calling process and mention that in documentation.