Skip to content

terminal: Stop the emulator first then kill the child process#45777

Closed
meipeter wants to merge 1 commit intozed-industries:mainfrom
meipeter:stop-the-emulater_fist-then-kill-the-child-process
Closed

terminal: Stop the emulator first then kill the child process#45777
meipeter wants to merge 1 commit intozed-industries:mainfrom
meipeter:stop-the-emulater_fist-then-kill-the-child-process

Conversation

@meipeter
Copy link

@meipeter meipeter commented Dec 28, 2025

Closes #44003
send shutdown to pty loop fist, then wait for 100ms send SIGKILL.
Release Notes:

@meipeter
send shutdown to pty loop fist, wait for 100ms then SIGKILL. This would
890d324 
fix zed-industries#44003 (not writing to shell history because the shell is force to
close) while ensure the shell will close.
@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Dec 28, 2025
@maxdeviant maxdeviant changed the title terminal:stop the emulater fist then kill the child process terminal: Stop the emulator first then kill the child process Dec 28, 2025
@meipeter
Copy link
Author

meipeter commented Dec 29, 2025

I GUESS there is maybe a better fix that is adding the timeout to SIGKILL in the alacritty crate's eventloop.As the alacritty using SIGHUP and .wait() the process exit.
I'm not familiar with Alacritty's code, so please forgive me if I'm misunderstanding the code.
line 306 in /tty/unix.rs in alacritty crate

impl Drop for Pty {
    fn drop(&mut self) {
        // Make sure the PTY is terminated properly.
        unsafe {
            libc::kill(self.child.id() as i32, libc::SIGHUP);
        }

        // Clear signal-hook handler.
        unregister_signal(self.sig_id);

        let _ = self.child.wait();
    }
}

@smitbarmase smitbarmase mentioned this pull request Jan 22, 2026
Merged
smitbarmase added a commit that referenced this pull request Jan 22, 2026
@smitbarmase @Veykril
terminal: Give child processes time to exit on their own (#47408)
76c8857 
Closes #44003

Supersedes #45777

Co-authored-by: Lukas <lukas@zed.dev>

Release Notes:

- Fixed an issue where the terminal would sometimes fail to write shell
history.

---------

Co-authored-by: Lukas Wirth <me@lukaswirth.dev>
@smitbarmase
Copy link
Member

smitbarmase commented Jan 22, 2026

Thanks for the PR, closing this in favor of #47408 which used delay from background executor instead of sleep.

zcg pushed a commit to zcg/zedpro that referenced this pull request Jan 29, 2026
@smitbarmase @Veykril
terminal: Give child processes time to exit on their own (zed-industr…
5c4eae1 
…ies#47408)

Closes zed-industries#44003

Supersedes zed-industries#45777

Co-authored-by: Lukas <lukas@zed.dev>

Release Notes:

- Fixed an issue where the terminal would sometimes fail to write shell
history.

---------

Co-authored-by: Lukas Wirth <me@lukaswirth.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@Veykril Veykril

Labels

cla-signed The user has signed the Contributor License Agreement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Fail to write to shell history

3 participants

@meipeter @smitbarmase @Veykril