[READY] Implement shutdown handler #2245

micbou · 2016-07-14T19:04:21Z

We don't want to block Vim exit if the server takes too much time to answer the shutdown request so we use a timeout of 100ms for this request. In my experience, it takes ~5ms.

The YcmStopServer command only exists for testing purpose. It will be removed in the final version of this PR.

Fixes #876.

This change is

Valloric · 2016-07-15T18:20:23Z

Nice!

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

python/ycm/youcompleteme.py, line 130 [r1] (raw file):

  def _SetupServer( self ):
    self._available_completers = {}
    self._user_notified_about_crash = False

Actually, this should be inside __init__. The idea is to initialize all member var inside __init__ if for any reason, then so that future readers have an easy way of knowing what are all the member variables used in this class.

Of course, there's nothing wrong with setting a different value to the member var in other functions, but they should be declared in __init__. (I use "declare" here loosely because there's no such concept in Python; values are merely assigned. Here by "declare" I mean "assigned for the first time.")

Any instances where we have member vars declared outside of __init__ are an oversight.

Comments from Reviewable

micbou · 2016-07-15T18:52:53Z

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

python/ycm/youcompleteme.py, line 130 [r1] (raw file):

Previously, Valloric (Val Markovic) wrote…

Actually, this should be inside __init__. The idea is to initialize all member var inside __init__ if for any reason, then so that future readers have an easy way of knowing what are all the member variables used in this class.

Of course, there's nothing wrong with setting a different value to the member var in other functions, but they should be declared in __init__. (I use "declare" here loosely because there's no such concept in Python; values are merely assigned. Here by "declare" I mean "assigned for the first time.")

Any instances where we have member vars declared outside of __init__ are an oversight.

I see but these variables need to be reset when restarting the server. If we initialize them inside `__init__`, we will set them twice to the same value when starting the server for the first time. Should we reset them in the `RestartServer` method instead?

Comments from Reviewable

Valloric · 2016-07-15T19:33:34Z

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

python/ycm/youcompleteme.py, line 130 [r1] (raw file):

we will set them twice to the same value when starting the server for the first time.

Why is this a problem?

I'm fine with setting the var to None in the ctor and then to False elsewhere if that helps. But setting it to False straight away still seems like the best choice.

Should we reset them in the RestartServer method instead?

Up to you. All I'd like to see is all member vars set to something in the ctor so that the reader can easily see what are all the member vars used in the class.

Comments from Reviewable

puremourning · 2016-07-16T15:54:54Z

Awesome.

with one comment.

Reviewed 7 of 9 files at r1, 3 of 4 files at r2.
Review status: 10 of 11 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

autoload/youcompleteme.vim, line 779 [r2] (raw file):

function! s:StopServer()
  py ycm_state.StopServer()

Does this work on py3 ? Isn't the command :py3 ?

IIRC we have a method or some script-local variable used to call the correct py command for PY2/PY3 ?

Comments from Reviewable

micbou · 2016-07-16T16:57:18Z

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

autoload/youcompleteme.vim, line 779 [r2] (raw file):

Previously, puremourning (Ben Jackson) wrote…

Does this work on py3 ? Isn't the command :py3 ?

IIRC we have a method or some script-local variable used to call the correct py command for PY2/PY3 ?

Oops, I added the command a long time ago before we bring Python 3 support. Fixed.

Comments from Reviewable

micbou · 2016-07-16T16:58:42Z

Reviewed 1 of 4 files at r2.
Review status: 10 of 11 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

python/ycm/youcompleteme.py, line 130 [r1] (raw file):

Previously, Valloric (Val Markovic) wrote…

we will set them twice to the same value when starting the server for the first time.

Why is this a problem?

I'm fine with setting the var to None in the ctor and then to False elsewhere if that helps. But setting it to False straight away still seems like the best choice.

Should we reset them in the RestartServer method instead?

Up to you. All I'd like to see is all member vars set to something in the ctor so that the reader can easily see what are all the member vars used in the class.

These two variables are now initialized in the constructor.

Comments from Reviewable

Valloric · 2016-07-16T20:59:59Z

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

.gitmodules, line 6 [r3] (raw file):

[submodule "third_party/ycmd"]
  path = third_party/ycmd
  url = https://github.com/micbou/ycmd

Don't forget to remove this. :)

Comments from Reviewable

puremourning · 2016-07-16T22:56:55Z

Looks good, though it looks like there's a problem with the CI... do you need a rebase, or is it flaking?

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

Comments from Reviewable

micbou · 2016-07-16T23:31:54Z

Yes, a rebase was needed.

Reviewed 5 of 9 files at r1, 3 of 4 files at r2, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

Use shutdown request to stop ycmd server in a portable way.

Create a Server_test class for tests that need a server instance.

micbou · 2016-07-23T11:15:52Z

Updated to latest ycmd. This is ready.

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

.gitmodules, line 6 [r3] (raw file):

Previously, Valloric (Val Markovic) wrote…

Don't forget to remove this. :)

Removed.

Comments from Reviewable

puremourning · 2016-07-23T11:45:10Z

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

Comments from Reviewable

vheon · 2016-07-23T13:18:08Z

@homu r=puremourning

Reviewed 5 of 9 files at r1, 1 of 4 files at r2, 1 of 1 files at r4, 3 of 4 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

homu · 2016-07-23T13:18:09Z

📌 Commit 4e94d7c has been approved by puremourning

homu · 2016-07-23T13:18:28Z

⚡ Test exempted - status

[READY] Implement shutdown handler This PR depends on ycm-core/ycmd#282. We don't want to block Vim exit if the server takes too much time to answer the shutdown request so we use a timeout of 100ms for this request. In my experience, it takes ~5ms. The `YcmStopServer` command only exists for testing purpose. It will be removed in the final version of this PR. Fixes #876.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2245)

micbou force-pushed the shutdown branch from f8394ee to 48d1557 Compare July 14, 2016 21:56

micbou force-pushed the shutdown branch from 8d77cc1 to 44d12e8 Compare July 16, 2016 15:09

micbou force-pushed the shutdown branch from 7027ff8 to ffe6b07 Compare July 16, 2016 22:58

micbou added 2 commits July 23, 2016 11:21

Use shutdown request to stop server

630b85a

Use shutdown request to stop ycmd server in a portable way.

Refactor tests using a server instance

7b2155a

Create a Server_test class for tests that need a server instance.

micbou force-pushed the shutdown branch from ffe6b07 to 7ee0d24 Compare July 23, 2016 09:23

Update to latest ycmd

4e94d7c

micbou force-pushed the shutdown branch from 7ee0d24 to 4e94d7c Compare July 23, 2016 09:30

micbou changed the title ~~[WIP] Implement shutdown handler~~ [READY] Implement shutdown handler Jul 23, 2016

homu merged commit 4e94d7c into ycm-core:master Jul 23, 2016

micbou deleted the shutdown branch October 1, 2016 15:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[READY] Implement shutdown handler #2245

[READY] Implement shutdown handler #2245

micbou commented Jul 14, 2016 •

edited

Valloric commented Jul 15, 2016

micbou commented Jul 15, 2016

Valloric commented Jul 15, 2016

puremourning commented Jul 16, 2016

micbou commented Jul 16, 2016

micbou commented Jul 16, 2016

Valloric commented Jul 16, 2016

puremourning commented Jul 16, 2016

micbou commented Jul 16, 2016

micbou commented Jul 23, 2016

puremourning commented Jul 23, 2016

vheon commented Jul 23, 2016

homu commented Jul 23, 2016

homu commented Jul 23, 2016

[READY] Implement shutdown handler #2245

[READY] Implement shutdown handler #2245

Conversation

micbou commented Jul 14, 2016 • edited

Valloric commented Jul 15, 2016

micbou commented Jul 15, 2016

Valloric commented Jul 15, 2016

puremourning commented Jul 16, 2016

micbou commented Jul 16, 2016

micbou commented Jul 16, 2016

Valloric commented Jul 16, 2016

puremourning commented Jul 16, 2016

micbou commented Jul 16, 2016

micbou commented Jul 23, 2016

puremourning commented Jul 23, 2016

vheon commented Jul 23, 2016

homu commented Jul 23, 2016

homu commented Jul 23, 2016

micbou commented Jul 14, 2016 •

edited