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.

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

Use detached: true on Windows #62

Merged
merged 2 commits into from
Nov 20, 2024
Merged

Use detached: true on Windows #62

merged 2 commits into from
Nov 20, 2024

Conversation

anaisbetts
Copy link
Contributor

Unfortunately, the fix in #55 was not sufficient - the behavior of spawn depends on the "console'ness" of the originally launched process, which has the effect of development behavior for Claude Desktop on Windows being different than in production, since the launching process is Claude.exe (a Win32 Subsystem app) vs yarn start which is effectively a Console Subsystem app.

Unfortunately, the fix in #55 was not sufficient - the behavior of spawn
depends on the "console'ness" of the originally launched process, which has the
effect of development behavior for Claude Desktop on Windows being
different than in production, since the launching process is Claude.exe
(a Win32 Subsystem app) vs `yarn start` which is effectively a Console
Subsystem app.
Copy link
Member

@jspahrsummers jspahrsummers left a comment

Choose a reason for hiding this comment

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

On Windows, setting options.detached to true makes it possible for the child process to continue running after the parent exits. The child process will have its own console window.

Both of these sound like the opposite of what we want, no? We definitely don't want servers to persist past the lifetime of their host application, and we're trying to eliminate its own console window.

@anaisbetts
Copy link
Contributor Author

anaisbetts commented Nov 20, 2024

Both of these sound like the opposite of what we want, no? We definitely don't want servers to persist past the lifetime of their host application, and we're trying to eliminate its own console window.

It appears that this documentation doesn't line up with the actual behavior (I suspect this behavior is describing node.exe which is inherently a Console Subsystem app where Electron in production is not; Electron itself also patches spawn)

Without detached, conhost.exe is attached:

CleanShot 2024-11-20 at 14 55 04@2x

With detached:

CleanShot 2024-11-20 at 14 56 41@2x

And we can see in Process Explorer that when Claude.exe exits, the node.exe does too

Copy link
Member

@jspahrsummers jspahrsummers left a comment

Choose a reason for hiding this comment

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

Okay, I trust you!

Can you add a comment to the code to this effect?

@anaisbetts anaisbetts merged commit 97cac5b into main Nov 20, 2024
4 checks passed
@anaisbetts anaisbetts deleted the ani/fix-dosbox-more branch November 20, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants