No lazy #2473

Merged
merged 2 commits into from Dec 15, 2016

Projects

None yet

6 participants

@wincent
Contributor
wincent commented Dec 14, 2016 edited

PR Prelude

  • I have read and understood YCM's CONTRIBUTING document.
  • I have read and understood YCM's CODE_OF_CONDUCT document.
  • I have included tests for the changes in my PR. If not, I have included a
    rationale for why I haven't.
  • I understand my PR may be closed if it becomes obvious I didn't
    actually perform all of these steps.

Why this change is necessary and useful

See prior version of this PR (#2454) for more discussion.


This change is Reviewable

wincent added some commits Dec 14, 2016
@wincent wincent Remove non-lazy lazy loading code
As discussed in Valloric#2454,
this code doesn't do what it claims to do on the surface, so we should
delete it.

I'll follow up with a commit to explain in the README how to actually
defer loading of the plug-in.
c22e425
@wincent wincent doc: Add FAQ entry explaining how to defer loading
7f76b03
@wincent wincent referenced this pull request Dec 14, 2016
Closed

Add g:ycm_start_autocmd option #2454

4 of 4 tasks complete
@codecov-io
codecov-io commented Dec 14, 2016 edited

Current coverage is 84.44% (diff: 100%)

Merging #2473 into master will not change coverage

@@             master      #2473   diff @@
==========================================
  Files            17         17          
  Lines          1903       1903          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           1607       1607          
  Misses          296        296          
  Partials          0          0          

Powered by Codecov. Last update 48b7cce...7f76b03

@Valloric
Owner

:lgtm:

Thanks for the PR!


Review status: 0 of 2 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@vheon
Collaborator
vheon commented Dec 15, 2016

:lgtm: @homu r=Valloric


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@homu
Collaborator
homu commented Dec 15, 2016

📌 Commit 7f76b03 has been approved by Valloric

@homu
Collaborator
homu commented Dec 15, 2016

⚡️ Test exempted - status

@homu homu merged commit 7f76b03 into Valloric:master Dec 15, 2016

7 checks passed

code-review/reviewable Review complete: 2 of 2 LGTMs obtained
Details
codecov/changes No unexpected coverage changes found.
Details
codecov/patch Coverage not affected when comparing 48b7cce...7f76b03
Details
codecov/project 84.44% (+0.00%) compared to 48b7cce
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test exempted
Details
@homu homu added a commit that referenced this pull request Dec 15, 2016
@homu homu Auto merge of #2473 - wincent:no-lazy, r=Valloric
No lazy

# PR Prelude

- [x] I have read and understood YCM's [CONTRIBUTING][cont] document.
- [x] I have read and understood YCM's [CODE_OF_CONDUCT][code] document.
- [x] I have included tests for the changes in my PR. If not, I have included a
  rationale for why I haven't.
- [x] **I understand my PR may be closed if it becomes obvious I didn't
  actually perform all of these steps.**

# Why this change is necessary and useful

See prior version of this PR (#2454) for more discussion.

[cont]: https://github.com/Valloric/YouCompleteMe/blob/master/CONTRIBUTING.md
[code]: https://github.com/Valloric/YouCompleteMe/blob/master/CODE_OF_CONDUCT.md

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2473)
<!-- Reviewable:end -->
c182c05
@Valloric Valloric added a commit that referenced this pull request Dec 16, 2016
@Valloric Revert "Auto merge of #2473 - wincent:no-lazy, r=Valloric"
This reverts commit c182c05, reversing
changes made to 48b7cce.
caa4291
@Valloric
Owner

OK, I just reverted this PR because it was causing gvim (but not console vim) to become unresponsive on Linux. Repro steps:

  1. Open gvim on linux (with this patch applied).
  2. Open third_party/ycmd/ycmd/utils.py (opening other files is likely to exhibit the same problem).
  3. Watch gvim get stuck forever and become unresponsive.

@wincent If you could figure out what's going on and how we could have this change without making gvim unhappy, we'd be willing to merge a new version of the change.

@micbou Thanks a bunch for noticing this!

@wincent
Contributor
wincent commented Dec 16, 2016

I don't have access to a Linux machine at this time, but if I can obtain access, I'll take a look.

@micbou
Collaborator
micbou commented Dec 16, 2016

This is caused by a bad interaction between Python's Popen and process forking on UNIX. Consider the following Python program:

import os
import subprocess

proc = subprocess.Popen( [ 'sleep', '1' ] )

pid = os.fork()

is_alive = proc.poll() is None
  
if pid:
    print( 'Parent process: {0}'.format( is_alive ) )
else:
    print( 'Child process: {0}'.format( is_alive ) )

It will output:

Parent process: True
Child process: False

As you can see, the child process incorrectly returns that the Popen process is terminated. The same thing is happening when loading YCM at gvim startup on Linux:

  1. YCM is loaded and the ycmd server is started through Popen;
  2. gvim forks its process (see :h gui-fork);
  3. the IsServerAlive method returns False because of the fork;
  4. YCM assumes that the server crashed and read its standard error. This blocks since the process is still running (the error stream is not closed) and thus gvim becomes unresponsive.

However, when loading YCM at VimEnter, gvim already did its fork and the poll() method from Popen works as expected.

In conclusion, I don't think we have much choice here; we can't load YCM at startup.

@vheon
Collaborator
vheon commented Dec 16, 2016

@micbou as always a great analysis!

In conclusion, I don't think we have much choice here; we can't load YCM at startup.

At least now we know :)

@wincent
Contributor
wincent commented Dec 16, 2016

Excellent explanation, @micbou.

@wincent wincent added a commit to wincent/YouCompleteMe that referenced this pull request Dec 17, 2016
@wincent wincent Don't use VimEnter initialization except when stating gui
This is another attempt at:

Valloric#2473

Which removed the apparently flawed attempt at "lazy" loading of YCM.
While it fails to get the load out of the critical startup path, it
*does* serve the useful purpose when starting up gvim of avoiding a
deadlock situation in gvim.

So, this time, we keep VimEnter, but only for the gui-starting case. We
update the comment to explain what is actually happening. And we can
keep the docs about how to defer loading.
1e3b7ba
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment