Remove waitForProcess in finalizer

[jneira]

It was causing hangs running test suites in Windows. Not sure why it was included, i've tried to make it works but only removing it did it

[lukel97]

This was added so that the servers can shut down themselves after the exit notification rather than relying on lsp-test to kill the process. I think it was added to address #70

Could that the mean that ghcide/hls aren't exiting properly on Windows?

[jneira]

Could that the mean that ghcide/hls aren't exiting properly on Windows?

I did not see any symptom of that using them, in lsp mode or console mode.

However, in the commit f2ff4d5, i see that the time out was changed from 1^6 to 10^6, did you test if only that change fixed the issue? Afaiu timeout msgTimeoutMs (runSession' exitServer) should give the server enough time to shutdown (with exitServer), once the timeout was fixed. The finally block should be a final resort, for when the server did not exit succesfully in that time, so that code block should stop it immediately and forcefully.

[lukel97]

@jneira Hm you're right, this is a bit confusing. The call to exitServer just sends the shutdown request and exit notification, so should be pretty fast. But I think that second block in the finally function is indeed meant to run every time, as we still need to stop the listener thread and wait for the server to terminate (timeout msgTimeoutMs (waitForProcess serverProc)). But if the server doesn't terminate, then that second timeout will kick in and skip it, moving onto the cleanupProcess server which should forcefully stop it.

If it's hanging on windows then that would mean that the timeout here isn't actually cancelling the waitForProcess serverProc? Is this a process related bug perhaps?

[lukel97]

I should point out that there is both waitForProcess and createProcess as we want to try and let the server exit normally first, before just force killing the server with createProcess. This PR would mean that it would always kill the server which would undo #70

[jneira]

If it's hanging on windows then that would mean that the timeout here isn't actually cancelling the waitForProcess serverProc? > Is this a process related bug perhaps?

Probably

Could we simply make the process sleep the timeout instead waitForProcess and then kill it? The effect would be the same.

[lukel97]

@jneira that sounds good, might make the tests a bit slower, but I'm also happy if you want to just to kill it if you can make it #ifdefed on the platform being Windows only

Out of curiosity though, does this only happen with local tests, or is there a particular branch it's hanging on? ghcides testsuite seems to be ok on Windows https://github.com/haskell/ghcide/runs/1128059426

[jneira]

Out of curiosity though, does this only happen with local tests, or is there a particular branch it's hanging on? ghcides testsuite seems to be ok on Windows https://github.com/haskell/ghcide/runs/1128059426

i am afraid that the test suite is not being executed for windows, note the --no-run-tests here: https://github.com/haskell/ghcide/blob/master/.azure/windows-stack.yml#L59 and the issue https://github.com/haskell/ghcide/issues/474

that sounds good, might make the tests a bit slower, but I'm also happy if you want to just to kill it if you can make it #ifdefed on the platform being Windows only

Yeah, it will wait after the process has finished , in most cases. After investigating the cause a little bit more, to find at least a bug report or something. Maybe i'will do both: sleep but only in windows :smiley_cat:

[jneira]

I think this issue is related woth this: https://github.com/haskell/process/issues/107 It is from 2017 😟 Not sure why it hangs, though.

[jneira]

Finally i opted to remove it entirely, with the hope that if it ends to be needed in windows, we will find a way to make it work

[lukel97]

Thanks! Do you want a release for this?

[jneira]

No at this moment, thanks!