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

Jump to bottom

virtual KPtyProcess::~KPtyProcess() the terminal process is still running, trying to stop it by SIGHUP #566

Open
tsujan opened this issue Nov 22, 2024 · 2 comments · May be fixed by #567
Open

virtual KPtyProcess::~KPtyProcess() the terminal process is still running, trying to stop it by SIGHUP #566

tsujan opened this issue Nov 22, 2024 · 2 comments · May be fixed by #567
Assignees
@tsujan

Comments

@tsujan
Copy link
Member

tsujan commented Nov 22, 2024 •
edited
Loading

The message of the title might happen repeatedly in the log (actually, each time a terminal window is closed) if I don't disable Qt warnings inside ~/.config/QtProject/qtlogging.ini. It's caused by these lines in the d-tor of KPtyProcess:

qtermwidget/lib/kptyprocess.cpp

Lines 96 to 104 in df7297b

waitForFinished(300); // give it some time to finish
if (state() != QProcess::NotRunning)
{
qWarning() << Q_FUNC_INFO << "the terminal process is still running, trying to stop it by SIGHUP";
::kill(static_cast<pid_t>(processId()), SIGHUP);
waitForFinished(300);
if (state() != QProcess::NotRunning)
qCritical() << Q_FUNC_INFO << "process didn't stop upon SIGHUP and will be SIGKILL-ed";
}

Qt doc says that QProcess::~QProcess() will not return until the process is terminated.

IMO, waiting for the finished() signal inside the d-tor of a QProcess isn't OK. I checked KDE's kpty → kptyprocess.cpp. They either never used waitForFinished() there or removed it a long time ago. Will use QTerminal without it for a while to test…
@tsujan
Copy link
Member Author

tsujan commented Nov 22, 2024

@palinek
This is a general question about QProcess. Any thought on it?

@tsujan
Copy link
Member Author

tsujan commented Nov 22, 2024 •
edited
Loading

Oh, it was added here 4 years ago, by @palinek: #171. Without it, this message is shown instead: QProcess: Destroyed while process ("/bin/bash") is still running.

Maybe another method needs to be found, because Konsole doesn't have this problem.

EDIT: Found a way by consulting Konsole's code.

@tsujan tsujan self-assigned this Nov 22, 2024
tsujan added a commit that referenced this issue Nov 22, 2024
@tsujan
Close the session correctly before destroying it
3ba543e 
Instead of trying to stop the process in the d-tor of `KPtyProcess`, the patch does it in the d-tor of `Session`.

I consulted Konsole's code — that didn't have our problem but was very different from our code — and adapted it to ours.

Closes #566
@tsujan tsujan linked a pull request Nov 22, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tsujan tsujan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Close the session correctly before destroying it lxqt/qtermwidget
1 participant
@tsujan