[READY] Prevent simultaneous starts of OmniSharp server

[micbou]

Currently, C# completer starts a new server on the FileReadyToParse event if no port is defined or the running server does not respond. This can lead to situations where multiple servers with same port and solution are started until one of them become ready. See PR #284 and ycm-core/YouCompleteMe#1913.

This is fixed by using ServerIsActive instead of ServerIsRunning to start the server in OnFileReadyToParse. We then check ServerIsRunning for an early return.

ServerIsActive is updated to check the handle (instead of the port) and to poll the process. We cannot only rely on the port because it can be defined while the process is down: server crashed during start up or its process was directly terminated by the user.

ServerIsRunning and ServerIsReady are also changed. We return early in both methods by checking ServerIsAlive first and we restrict exceptions catching.

[Valloric]

with minor comments.

---

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

ycmd/completers/cs/cs_completer.py, line 235 [r1] (raw file):
It would be a good idea to add a comment here or on the above IF explaining what's going on. It's not really obvious from the code.

---

ycmd/completers/cs/cs_completer.py, line 555 [r1] (raw file):
The fact that poll() returns None when process is still running is confusing as hell. Let's create wrapper ProcessStillRunning(popen_handle) in utils that returns bool and encapsulates this logic.

In fact, the wrapper can also handle popen handle being None too, so this check only returns the value from the helper func.

---

Comments from the review on Reviewable.io

[vheon]

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

ycmd/completers/cs/cs_completer.py, line 231 [r1] (raw file):
wouldn't be better to flip those conditions? why check if the server is active if then we don't want to start a new one anyway?

---

Comments from the review on Reviewable.io

[micbou]

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.

---

ycmd/completers/cs/cs_completer.py, line 231 [r1] (raw file):
Done.

---

ycmd/completers/cs/cs_completer.py, line 235 [r1] (raw file):
Done.

---

ycmd/completers/cs/cs_completer.py, line 555 [r1] (raw file):
Done.

---

Comments from the review on Reviewable.io

[puremourning]

Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

ycmd/completers/javascript/tern_completer.py, line 443 [r2] (raw file):
Requires argument!

This must mean we're not covering this in the tests? :/

---

Comments from the review on Reviewable.io

[micbou]

Review status: 2 of 3 files reviewed at latest revision, 4 unresolved discussions.

---

ycmd/completers/javascript/tern_completer.py, line 443 [r2] (raw file):
No, this is covered but I only ran the tests for C♯. Dumb me.

---

Comments from the review on Reviewable.io

[puremourning]

LGTM assuming the tests pass :)

---

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

ycmd/completers/javascript/tern_completer.py, line 443 [r2] (raw file):
Hah no problem. Team work.

---

Comments from the review on Reviewable.io

[vheon]

---

Reviewed 2 of 3 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

Comments from the review on Reviewable.io

[Valloric]

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

ycmd/completers/cs/cs_completer.py, line 585 [r3] (raw file):
ServerTerminated seems to be the exact inverse of ServerIsActive, both logically and implementation-wise. Do we need both then? Can we kill one? If we can, let's keep the positive ServerIsActive.

---

Comments from the review on Reviewable.io

[micbou]

Review status: 2 of 4 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

ycmd/completers/cs/cs_completer.py, line 585 [r3] (raw file):
I removed it and use ServerIsActive instead.

---

Comments from the review on Reviewable.io

[Valloric]

I still see [WIP] in the issue heading. Is there some work still pending?

---

Review status: 2 of 4 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

Comments from the review on Reviewable.io

[vheon]

Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

Comments from the review on Reviewable.io

[micbou]

Yes, I changed the name of ServerIs* methods to correspond to ycmd handlers /healthy and /ready. I also simplified these methods (request_data parameter was never used). I think it makes more sense now.

---

Review status: 2 of 5 files reviewed at latest revision, 1 unresolved discussion.

---

Comments from the review on Reviewable.io

[puremourning]

Reviewed 3 of 3 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from the review on Reviewable.io

[vheon]

Reviewed 3 of 3 files at r5.
Review status: 4 of 5 files reviewed at latest revision, 1 unresolved discussion.

---

Comments from the review on Reviewable.io

[Valloric]

All good, thank you! :)

@homu r+

---

Reviewed 1 of 3 files at r2, 1 of 1 files at r3, 2 of 3 files at r5, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[homu]

📌 Commit 940448a has been approved by Valloric

[homu]

⌛ Testing commit 940448a with merge 92bc86e...

[homu]

☀️ Test successful - status

[coveralls]

Coverage increased (+12.9%) to 95.122% when pulling 940448a on micbou:cs-completer into f23e372 on Valloric:master.