Detach session when forced close detected (handling SIGTERM and other signals) #581
Conversation
dantepippi
changed the title
zellij-client/src/lib.rs
Outdated
| .send(ClientInstruction::Exit(ExitReason::Normal)) | ||
| .send(ClientInstruction::Exit(ExitReason::ForceDetached)) |
There was a problem hiding this comment.
Does this behave as expected?
I think we should rather be sending ClientToServerMsg::Action(Action::Detach) to the server for the actual detach to happen.
(Even the existing logic seems suspicious to me. We should have been sending ClientToServerMsg::Action(Action::Quit) for a clean exit. Must have missed changing this part when the refactor happened.)
Edit: Currently in main, both client and server stay alive in the background even though both should exit after the terminal closes. So the existing logic was wrong.
There was a problem hiding this comment.
Hey @kunalmohan! In my tests with this code I was able to force close zellij (with interrupt signals) and see the session available with list-sessions - that didn't happen with main. But I'm ok with doing some refactoring.
On a related note, what do you think about changing the behavior for zellij attach so instead of an error if there are no detached sessions, it would start a new one. What I'm thinking is to always start zellij with the attach option so I recover some eventually detached session when available or create a new one otherwise. Maybe recovering the previous session should even be the default.
There was a problem hiding this comment.
I believe it is being worked on in #541. The idea is to have a --create (-c) flag with the attach command to create a new session if one does not already exist. I'm not sure about making it default. Attaching to an existing session and creating a new one are two separate operations. Attach command having a default behaviour of "attach or create" sounds a bit weird and confusing. IMO having a config option to change the default behaviour of this command can be a good option.
There was a problem hiding this comment.
But that would involve me knowing the name of a session to use with --create (session-name). My suggestion was more in the line that if a user has a detached session (because it was previously interrupted) when he opens Zellij again he probably wants to continue from where he left. So it would be nice for UX not having to know that session name. If you have just one session detached "recover" it instead of creating new sessions.
So the proposal would be to change zellij attach to attach if there is one session available (whatever that name might be) or create a new one otherwise.
There was a problem hiding this comment.
How about zellij a -c? The session name is already optional for attach command, so in case of zellij a -c:
- If only single session exists, recover it.
- If more than one sessions exists, exit with a message (as we do now).
- If no session exists, create a new one with a random name.
There was a problem hiding this comment.
Re: Whether zellij attach should have a separate --create flag or a default behaviour of "recover-or-create". I'm inclined towards the former, but don't have a strong opinion about it. I'm guessing that other folks might also have an opinion about this, so maybe we can move this discussion to discord or a new issue? We can then decide accordingly.
There was a problem hiding this comment.
Thanks @kunalmohan ! I honestly would be happy with both scenarios (default or not) as long as we have the option to "recover or create" without requiring a name to recover.
Yes, lets continue this on Discord.
Auto detach the session when receiving a forced close signal (SIGWINCH, SIGTERM, SIGINT, SIGQUIT, SIGHUP).
The current behavior when the user manually closes Zellij (ctrl-q) remains the same and does not auto detach.
Closes #572