Skip to content
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.

Sign up for GitHub

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

Detach session when forced close detected (handling SIGTERM and other signals) #581

Merged
merged 2 commits into from
Jun 19, 2021
Merged

Detach session when forced close detected (handling SIGTERM and other signals) #581

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
18adbcc
Detach session on a forced close (handlign SIGTERM and other signals)
dantepippi Jun 18, 2021
e7fc0e3
Sending the action Action::Detach to the server
dantepippi Jun 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion zellij-client/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ pub fn start_client(
let send_client_instructions = send_client_instructions.clone();
move || {
send_client_instructions
.send(ClientInstruction::Exit(ExitReason::Normal))
.send(ClientInstruction::Exit(ExitReason::ForceDetached))
Copy link
Member

@kunalmohan kunalmohan Jun 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dantepippi dantepippi Jun 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about zellij a -c? The session name is already optional for attach command, so in case of zellij a -c:

  1. If only single session exists, recover it.
  2. If more than one sessions exists, exit with a message (as we do now).
  3. If no session exists, create a new one with a random name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.unwrap()
}
}),
Expand Down