ssh: start monitor ConnPid before casting socket_control #8310
Conversation
CT Test Results 2 files 29 suites 18m 31s ⏱️ Results for commit 211ce03. ♻️ 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 |
|
I am not sure why one of the tests is failing but it seems unrelated to the changes done in this PR. |
Looks like some unrelated environment issue. Happens sometimes on github. |
|
we will stall this one until #8226 is handled |
| @@ -129,12 +129,13 @@ takeover(ConnPid, _, Socket, Options) -> | |||
| {_, Callback, _} = ?GET_OPT(transport, Options), | |||
| case Callback:controlling_process(Socket, ConnPid) of | |||
| ok -> | |||
| Ref = erlang:monitor(process, ConnPid), | |||
There was a problem hiding this comment.
Agree that the code seems to expect a monitor here. I however think it would be more natural to implement this with a gen_statem call and a server side timeout (if a timeout is needed)!
There was a problem hiding this comment.
I think I would like this small change to be merged and patched on supported versions.
@IngelaAndin comment will remain relevant and should be included in planned ssh supervison tree update (I've made a note about that)
|
@Maria-12648430 did you see this one, I am hoping that this one will no longer be necessary ones we get a new solution to |
Yes, I saw. Turning this into a call is a bit complicated though, since the statem does a lot of things from this point onward before it replies (like, it changes the callback module a few times). But I'll keep it in mind 👍 |
We should start monitoring ConnPid before
gen_statem:cast(ConnPid, socket_control). It's possible that ConnPid is closed when handling thesocket_controlcast. If we are unlucky, ConnPid is closed before we start monitoring leading to anoprocbeing returned from the monitor instead of the actual error.I had a planned to write a small unit test to test my changes but without meck it will be difficult..