Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limit the number of unanswered Typings Installer requests #18265

Merged
merged 4 commits into from
Sep 7, 2017

Conversation

amcasey
Copy link
Member

@amcasey amcasey commented Sep 6, 2017

If we send them all at once, we (apparently) hit a buffer limit in the Node IPC channel and both TS Server and the Typings Installer become unresponsive.

We accomplish this by explicitly maintaining a queue of yet-to-be-sent requests.

Fixes #18255

If we send them all at once, we (apparently) hit a buffer limit in the
node IPC channel and both TS Server and the typings installer become
unresponsive.
@msftclas
Copy link

msftclas commented Sep 6, 2017

@amcasey,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

@minestarks
Copy link
Member

Fixes #18255

@minestarks
Copy link
Member

minestarks commented Sep 6, 2017

Some thoughts about massive projects

  • I'm worried about the magic limit 10 not being enough to protect against the buffer being blown. I think the project we were investigating might have had less than 10 projects. Perhaps we could have a limit on total size of in-progress requests: we could use a heuristic for the size such as request.fileNames.length?

    • Edit: I'm having second thoughts about this constant limit approach altogether. We (I, at least) don't really have much insight on the underlying buffer yet, and I'm not confident it's going to be the same size across different hardware, Node releases, etc. At the very least, I think we should handle the case where we still fill the buffer, despite our mitigations (instead of just getting stuck). I'll try to understand how we could do this a little better tomorrow.
  • On a related note, do we still queue a typings request for a project we've disabled as a result of the 20MB limit? If so, it should be a no-brainer to quit doing that.

@amcasey
Copy link
Member Author

amcasey commented Sep 6, 2017

Failures are lint. I'll update.

@amcasey
Copy link
Member Author

amcasey commented Sep 7, 2017

@minestarks Are disabled projects flagged in some way?

@minestarks
Copy link
Member

languageServiceEnabled

@amcasey
Copy link
Member Author

amcasey commented Sep 7, 2017

It does not appear to be skipped at present. I'd like to fix that in a separate PR though.

@amcasey amcasey merged commit 8d1eb29 into microsoft:master Sep 7, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Sep 7, 2017

@amcasey can you port this to release-2.5

@amcasey
Copy link
Member Author

amcasey commented Sep 7, 2017

@mhegazy we're just waiting on final validation from an affected user.

@amcasey amcasey deleted the ThrottledOperations branch September 7, 2017 23:34
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants