Fix non terminating tests in Windows #44

jneira · 2019-07-09T12:54:51Z

The pr basically ensures the Exit message is sent to server process before killing its listener thread to shutdown it gracefully.
As commented in a previour pr in windows you cant kill a thread that is blocked reading from a Handler (like the server listener is) so the failing tests were blocked in the killThread call.
I had to convert a raw error decoding an empty response from server in a SessionException to avoid throwing it when server had been exited previously.
- Using the IORef exitOk hack previously in LSP.Test
- Ideally we should have a nicer way to know if a server is running or had been exited, saving its state in SessionContext or SessionState or using some ack mechanism with messages, but it could be done in another pr

Hopefully this would fix haskell/haskell-ide-engine#713

It is the first one that doesnt terminate in windows

…ver is down

jneira · 2019-07-10T04:31:11Z

I have to take a look, the tests are failing or showing a stacktrace intermittently

jneira · 2019-07-10T05:41:22Z

It seems in windows 7 (my original test env) it works fine, but in windows 10 doesnt.

jneira · 2019-07-10T22:17:41Z

Ok, i had to use the shutdown step with its ack to break the server listener loop (sorry for the imperative jargon 😆 )
I am afraid that if the server becomes unresponsive this will not work and the hie process will not terminate in windows. To ensure that the raw killThread will continue being called in non-windows systems i've used finally and timeout.

jneira · 2019-07-11T08:18:35Z

Another take: to make sure we can kill the server listener thread i've forced the kill of server process before
The lsp test-suit pass with that change
The func-test suite of hie terminates for windows (see output)
I' ve skipping the manual javascript session passes a test test in travis for now to make pass the windows ci build

jneira · 2019-07-11T08:36:33Z

test/Test.hs

@@ -104,7 +104,7 @@ main = hspec $ do

      it "don't throw when no time out" $ runSessionWithConfig (def {messageTimeout = 5}) "hie" fullCaps "test/data/renamePass" $ do
        loggingNotification
-        liftIO $ threadDelay 10
+        liftIO $ threadDelay $ 10 * 1000000


@bubba Not sure it it was done intencionally, but test did not terminate in my windows 7 with 10

Yes, I presume I meant to write 10s not 10us!

lukel97 · 2019-07-11T10:34:12Z

Thanks so much for helping out with this. I’ll take a closer look at this later this week

jneira · 2019-07-11T10:45:07Z

The test of manual js session is failing for linux cause i am trying now to decode effectively the response to examine if it is a RspShutdown and the js server is returning:

{"jsonrpc":"2.0", "id":0,"result":null}

instead the valids for the Aeson encoding

{"jsonrpc":"2.0", "id":0} -> ResponseMessage { _jsonrpc ="2.0", _id =1, _result = Nothing}

or

{"jsonrpc":"2.0", "id":0, "result"=""} -> ResponseMessage { _jsonrpc ="2.0", _id =1, _result = ""}

jneira · 2019-07-11T12:49:48Z

src/Language/Haskell/LSP/Test/Decoding.hs

@@ -120,6 +120,7 @@ matchResponseMsgType req = case req of
  where decoded x = fromMaybe (error $ "Couldn't decode response for the request type: "
                                        ++ show req ++ "\n" ++ show x)
                              (decode x)
+        removeNullResult x = maybe x (<> "}") (B.stripSuffix ",\"result\":null}" x)


An ugly hack to sanitize the response from the manual javascript session
Maybe we should have a generic way to remove fields with null values or change the json encoding to make field:null = _field=Nothing

I wouldn't worry about the javascript test too much. This looks like its caused by how haskell-lsp doesn't parse responses with null results (which should probably be fixed at some point). You can just revert this and let the javascript test fail for now

lukel97 · 2019-07-11T20:15:58Z

src/Language/Haskell/LSP/Test.hs

+
+-- | Exit the server after request its shutdown
+exitServer :: Session()
+exitServer = request_ Shutdown (Nothing :: Maybe Value) >> sendNotification Exit ExitParams


Does this need to be exposed to the API? The server should automatically be exited once the withSession closure finishes

I had to expose it cause it is used in Language.Haskell.LSP.Test.Replay.
I think it should be in Session or in a module upstream, like the server listener handler (their clean up is closely related), and reduce the number of params of runSessionWithHandles but it would need move quite code (about requests/reponses) from Test to another module on top of Session, due to circular dependencies.

lukel97 · 2019-07-11T20:19:27Z

src/Language/Haskell/LSP/Test/Decoding.hs

@@ -120,6 +120,7 @@ matchResponseMsgType req = case req of
  where decoded x = fromMaybe (error $ "Couldn't decode response for the request type: "
                                        ++ show req ++ "\n" ++ show x)
                              (decode x)
+        removeNullResult x = maybe x (<> "}") (B.stripSuffix ",\"result\":null}" x)


I wouldn't worry about the javascript test too much. This looks like its caused by how haskell-lsp doesn't parse responses with null results (which should probably be fixed at some point). You can just revert this and let the javascript test fail for now

lukel97 · 2019-07-11T20:20:41Z

I just cleared the windows cache on travis, I've seen that error pop up some times when using the wrong version of hie. Once there's a successful build of windows it should (hopefully) run a lot quicker

This reverts commit 6f031f5.

jneira · 2019-07-12T08:09:31Z

I bumped up the version of hie to 0.11.0.0 but the same tests are failing in windows.
I swear it works on my machine! 🤣

jneira · 2019-07-12T10:56:12Z

I've executed the hie tests using this pr version of lsp-test in the azure ci and i am afraid the user doesn't have permissions to terminate proccesses 😟

Warming up HIE cache...
HIE cache is warmed up
func-test.exe: terminateProcess: permission denied (Permission denied)

jneira · 2019-07-12T11:28:22Z

Otoh i've set up an appveyor build for lsp-test itself using stack (like i am using in local) and the test suite passed: https://ci.appveyor.com/project/jneira/lsp-test/builds/25932605

In a second build i've got the same erorrs than travis one: https://ci.appveyor.com/project/jneira/lsp-test/builds/25934347

jneira · 2019-07-15T22:54:58Z

Although i am able to run the test suite of lsp-test and hie succesfully using this pr i am worried by the ci results

The builds of hie in azure (the next official ci) are not stable for windows, some of them terminate as expected (with 16 legitimate errors) but others doesnt terminate or exits with terminateProcess: permission denied (see https://dev.azure.com/jneira/haskell-ide-engine/_build/results?buildId=99 in which we can see the three cases in the same build). In linux and macos it fails as expected, like before this pr.
I cant get to run a new succesfull build of lsp-test in appveyor or travis since this one. I ve tried to clear the cache, different hie commits...

jneira · 2019-07-16T21:49:10Z

The tests are a little bit more clear:

I've trigged a travis build with the original hie commit and it seems it will go better (linux build is ok, mac os failed by timeout at the end and windows continue failing with 4 fails)
The different errors trying the hie func-test were because i forgot to use the pr in all stack-*.yaml. In fact there is only one:
- terminateProcess: permission denied in windows build with ghc < 8.6. Here i think we are hitting non deterministic exception from withCreateProcess on Windows: "terminateProcess: permission denied (Permission denied)" haskell/process#110, resolved in process-1.6.5.0 only available in those ghc versions
- Otoh we can't use cleanupProcess directly cause it is only available since process-1.6.4.0. We'll need to use interruptProcess and hClose directly.

It's not needed since the captured session should contain it anyway

lukel97 · 2019-07-20T19:54:26Z

I've tidied up the exitServer stuff, it shouldn't be needed by the replay module since the captured file should be replaying back the exit request and notification anyway. This PR passes tests for me locally (except for the javascript one, but I've created a PR to fix that haskell/lsp#181) so I'm happy to merge this if you are?

jneira · 2019-07-20T23:08:25Z

@bubba well, i am little bit worried for the hie tests in azure-ci (https://dev.azure.com/jneira/haskell-ide-engine/_build/results?buildId=167):

some of them are failing at compile time due to a error in cpp conditions of Compat (currently fixing)
other continues failing with terminateProcess: permission denied: seems to affects all ghc versions with process < 1.6.4.0
- i've just reproduced in local with cabal v2-test -w "D:\bin\stack\x86_64-windows\ghc-8.4.2\bin\ghc"
Sometimes the case withTimeout > times out fails. I still have to reproduce the error and find the cause.

The first one is easy to fix but not sure about the second, the fix in process involved changing c code.
I would like try to make a workaround a little more time but we `ll have to decide what to do if we do not get it.

…to fix-win-tests

jneira · 2019-07-22T05:55:41Z

I have some problems with cpp conditionals in Compat. It turns out that cleanupProcess is in the public api of process since 1.6.4.0 but the 1.6.3.0 version shipped with ghc 8.6.1 and 8.6.2 has it in the public api (see haskell-ide-engine build in https://dev.azure.com/jneira/haskell-ide-engine/_build/results?buildId=178)
So i am afraid i have to hide it for 1.6.3.0 although it causes warnings about dodgy imports with the official 1.6.3.0 release of hackage and the versions shipped with ghc 8.4.*.

jneira · 2019-07-22T10:50:56Z

I've need to add ignoresigPipe to the Compat version of cleanupProcess or terminate process kept hanging.
@bubba I've added two stack.yaml files for the lastest minor versions of ghc used in hie, cause they have helped in fixing issues with different version of the process package. Maybe it should belong to another pr, let me know if it is the case and i will delete them.

jneira · 2019-07-24T05:56:35Z

Ok, i think it could be the last needed change after a bit of yak shaving with process.
To avoid the issue with permission denied i've decided to copy withServer for process <= 1.6.4.0 to make it use the local version of cleanupProcess with the low level IOError ignored.

I think it is not the correct solution in the general case, cause we are ignoring legitimate permission denied errors in windows and other os's but i think it could work just enough for our case.

Running the hie func-test in azure using this version of lsp-test: https://dev.azure.com/jneira/haskell-ide-engine/_build/results?buildId=185

jneira · 2019-07-24T08:55:35Z

@bubba ok, i think this is done, tests in azure build have failed correctly 😆
The build on travis has failed for linux and macos due to timeout.

jneira · 2019-07-24T09:46:44Z

As process-1.6.4.0 was not shipped with any ghc and stack doesnt let use it, i've tested locally cabal v2-run lsp-test:tests:test --constraint process==1.6.4.0 with success.

lukel97

Tested this out locally and it works great on mac, and the window stuff seems to be in a lot better of a state than it was previously. Thanks for the work on this!

cocreature · 2019-08-09T11:43:51Z

@jneira Thanks a lot for cleaning up the mess that I produced and fixing it!

Fix non terminating tests in Windows

jneira added 7 commits July 8, 2019 14:11

Add "rename suggestion" test

5fed475

It is the first one that doesnt terminate in windows

Shutdown the server before kill its thread

5bc242f

Remove comment about IORef

c922377

Add server shutdown check to throw exception

a71684d

Add new NoContentLengthHeader SessionException to replace raw error

94e25cb

Use a SessionException instead an error, to ignore the error when ser…

cecfb20

…ver is down

Remove test that already exists in hie

0f4d99b

jneira mentioned this pull request Jul 9, 2019

Get tests working on windows haskell/haskell-ide-engine#713

Closed

jneira changed the title ~~Fix win tests~~ [WIP] Fix win tests Jul 10, 2019

jneira added 2 commits July 10, 2019 12:35

Exit the server and its listener properly

bd2ac7d

Add a finally and timeout to ensure the call to killThread

fa27834

jneira added 3 commits July 11, 2019 10:21

Use the same magnitude in session timeout and threadDelay

b00bc72

Use ProcessHandle in withServer to allow kill it by client code

1b1df64

Skip manual javascript session passes a test for now in windows

a75d911

jneira commented Jul 11, 2019

View reviewed changes

Avoid error when trying to shutdown manual js session

6f031f5

jneira commented Jul 11, 2019

View reviewed changes

lukel97 reviewed Jul 11, 2019

View reviewed changes

Revert "Avoid error when trying to shutdown manual js session"

07870d4

This reverts commit 6f031f5.

jneira changed the title ~~[WIP] Fix win tests~~ Fix non terminating tests in Windows Jul 11, 2019

jneira added 3 commits July 12, 2019 07:35

Checkout lastest release version of hie in travis

dff04d2

Skipping manual js session test

07f9565

Checkout using commit instead a tag

8a86e8d

jneira added 2 commits July 16, 2019 13:16

Replace cleanupServer with functions avalilable in ghc <= 8.4

1c03309

Use original hie commit

0c53a7b

jneira and others added 3 commits July 17, 2019 14:57

Add cleanupRunningProcess to Compat module

b9125da

Merge branch 'fix-win-tests-compat' into fix-win-tests

02c7c7c

Don't use exitServer in Replay

f0a9615

It's not needed since the captured session should contain it anyway

jneira added 2 commits July 21, 2019 01:35

Fix conditionals for process-1.6.3.0

c20539b

Merge branch 'fix-win-tests' of https://github.com/jneira/lsp-test in…

f14d6c8

…to fix-win-tests

jneira added 3 commits July 22, 2019 08:07

Hide cleanupProcess for process-1.6.3.0 with comment

2477907

Include ignoreSigPipe to avoid hangs

e76f9e6

Add stack.yaml for last minor versions used in hie

0c42526

jneira added 2 commits July 23, 2019 14:55

Ignore 'permisision denied' error cleaning up processes

d7dc402

Use the correct errno and remove cleanupRunningProcess

ed60503

Remove extra stack files

6936320

lukel97 approved these changes Aug 6, 2019

View reviewed changes

lukel97 merged commit d126623 into lukel97:master Aug 6, 2019

jneira deleted the fix-win-tests branch August 9, 2019 12:33

lukel97 added a commit to haskell/lsp that referenced this pull request Feb 27, 2021

Merge pull request lukel97/lsp-test#44 from jneira/fix-win-tests

343650b

Fix non terminating tests in Windows

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix non terminating tests in Windows #44

Fix non terminating tests in Windows #44

jneira commented Jul 9, 2019 •

edited

jneira commented Jul 10, 2019

jneira commented Jul 10, 2019

jneira commented Jul 10, 2019 •

edited

jneira commented Jul 11, 2019 •

edited

jneira Jul 11, 2019

lukel97 Jul 11, 2019

lukel97 commented Jul 11, 2019

jneira commented Jul 11, 2019

jneira Jul 11, 2019

lukel97 Jul 11, 2019

lukel97 Jul 11, 2019

jneira Jul 11, 2019 •

edited

lukel97 Jul 11, 2019

lukel97 commented Jul 11, 2019

jneira commented Jul 12, 2019

jneira commented Jul 12, 2019 •

edited

jneira commented Jul 12, 2019 •

edited

jneira commented Jul 15, 2019 •

edited

jneira commented Jul 16, 2019 •

edited

lukel97 commented Jul 20, 2019

jneira commented Jul 20, 2019 •

edited

jneira commented Jul 22, 2019 •

edited

jneira commented Jul 22, 2019 •

edited

jneira commented Jul 24, 2019 •

edited

jneira commented Jul 24, 2019

jneira commented Jul 24, 2019 •

edited

lukel97 left a comment

cocreature commented Aug 9, 2019

Fix non terminating tests in Windows #44

Fix non terminating tests in Windows #44

Conversation

jneira commented Jul 9, 2019 • edited

jneira commented Jul 10, 2019

jneira commented Jul 10, 2019

jneira commented Jul 10, 2019 • edited

jneira commented Jul 11, 2019 • edited

jneira Jul 11, 2019

Choose a reason for hiding this comment

lukel97 Jul 11, 2019

Choose a reason for hiding this comment

lukel97 commented Jul 11, 2019

jneira commented Jul 11, 2019

jneira Jul 11, 2019

Choose a reason for hiding this comment

lukel97 Jul 11, 2019

Choose a reason for hiding this comment

lukel97 Jul 11, 2019

Choose a reason for hiding this comment

jneira Jul 11, 2019 • edited

Choose a reason for hiding this comment

lukel97 Jul 11, 2019

Choose a reason for hiding this comment

lukel97 commented Jul 11, 2019

jneira commented Jul 12, 2019

jneira commented Jul 12, 2019 • edited

jneira commented Jul 12, 2019 • edited

jneira commented Jul 15, 2019 • edited

jneira commented Jul 16, 2019 • edited

lukel97 commented Jul 20, 2019

jneira commented Jul 20, 2019 • edited

jneira commented Jul 22, 2019 • edited

jneira commented Jul 22, 2019 • edited

jneira commented Jul 24, 2019 • edited

jneira commented Jul 24, 2019

jneira commented Jul 24, 2019 • edited

lukel97 left a comment

Choose a reason for hiding this comment

cocreature commented Aug 9, 2019

jneira commented Jul 9, 2019 •

edited

jneira commented Jul 10, 2019 •

edited

jneira commented Jul 11, 2019 •

edited

jneira Jul 11, 2019 •

edited

jneira commented Jul 12, 2019 •

edited

jneira commented Jul 12, 2019 •

edited

jneira commented Jul 15, 2019 •

edited

jneira commented Jul 16, 2019 •

edited

jneira commented Jul 20, 2019 •

edited

jneira commented Jul 22, 2019 •

edited

jneira commented Jul 22, 2019 •

edited

jneira commented Jul 24, 2019 •

edited

jneira commented Jul 24, 2019 •

edited