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

jdtls python script forks on Windows #2615

Closed
Tachi107 opened this issue Apr 23, 2023 · 6 comments · Fixed by #2654
Closed

jdtls python script forks on Windows #2615

Tachi107 opened this issue Apr 23, 2023 · 6 comments · Fixed by #2654
Assignees
Milestone

Comments

@Tachi107
Copy link

Tachi107 commented Apr 23, 2023

Hi, first of all thanks for this language server! It makes writing Java code on lightweight editors like Vim way less painful :)

The jdtls.py script is currently unusable in certain scenarios on Windows since os.execvp forks and runs the server in a child proces, and terminates the python interpreter. Plugins watching the python3 jdtls process see that the process is exited, and report that the language server has exited too, while it's still running as a child process. This is the case for the vim9 LSP plugin, for example; see this issue: yegappan/lsp#285

Wouldn't it be possible to move most of the logic into the java program itself? This would make using the plugin in such situations way easier.

Thanks!

@rgrunber
Copy link
Contributor

rgrunber commented Apr 24, 2023

os.execvp was replaced in #2048 . Seems like reverting that patch is one way to guarantee that the script itself stays up. I assume it was simpler though to simply terminate the python script since it wasn't doing anything useful after it launched JDT-LS.

@jreicher , other than simplicity, what's the reason for terminating the python script and re-parenting JDT-LS under the process that launched the script ?

@williamboman
Copy link

FWIW, I can confirm that using subprocess.run fixes the issue on Windows.

@rgrunber
Copy link
Contributor

rgrunber commented Apr 28, 2023

CC: @schrieveslaach for comment as well. Looking into the history, we went from os.system (which guaranteed the JDT-LS process would be a child of the process that launched the python script) -> subprocess.run -> os.execvp.

The proposal here is to go back to subprocess.run since some clients may want to use the state of the python script launch process to determine if the language server remains up or not.

I would think the client that launches the python script has probably set up communication with the LS, so it should create some API to report the state of the LS back to other extensions that might need that info, but for now I'm definitely open to subprocess.run

@williamboman
Copy link

Maybe a good compromise would be to only use subprocess.run on Windows systems, and execvpe elsewhere 🤷‍♂️? Can't say I'm aware of good job mgmt practices on Windows, but it'd at least makes the script functional on it.

@jreicher
Copy link
Contributor

To be honest I assumed execvp would behave "in the usual manner" on Windows, potentially making subprocess control more robust by removing the python process as an intermediary in process status and signal passing

But the fact it spawns a new process defeats this purpose (and seems quite ridiculous to me)

A bit of googling suggests I'm not the only one surprised by this and that using subprocess.run() is a fairly common compromise

@JessicaJHee
Copy link
Contributor

I'm interested in taking a look at this!

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 a pull request may close this issue.

5 participants