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

Fix truncated stream #477

Merged
merged 2 commits into from
Jan 16, 2021
Merged

Fix truncated stream #477

merged 2 commits into from
Jan 16, 2021

Conversation

krassowski
Copy link
Member

@krassowski krassowski commented Jan 16, 2021

References

Fixes #450, enabling parsing of large responses from R server (e.g. completion on base) and any responses from javascript/typescript language servers

Code changes

Turns out BufferedIOBase.read() does buffering... And we need to read in chunks until we consume all needed chunks.

User-facing changes

Completion for R works better, completion for javascript works. May also help some Python packages with huge namespaces, e.g numpy. It will also improve perceived completion speed.

long_namespace_completion_works

Backwards-incompatible changes

Chores

  • linted
  • tested - TODO a robot test for R base completion.
  • documented
  • changelog entry

@krassowski krassowski merged commit f111e6b into master Jan 16, 2021
@bollwyvl
Copy link
Collaborator

bollwyvl commented Jan 16, 2021 via email

@krassowski
Copy link
Member Author

krassowski commented Jan 24, 2021

It did not solve the issue on Windows which is slightly more complicated there.

On Windows we get OSError: [Errno 22] Invalid argument which seems to be coming from the non-blocking stream implementation (https://stackoverflow.com/questions/34504970/non-blocking-read-on-os-pipe-on-windows/34504971#34504971). Do we need the stream to be non-blocking? Should we rewrite it using tornado.iostream?

Also, the non-blocking behaviour has issues in Python: https://bugs.python.org/issue13322

Edit: removing the non-blocking mode fixes the issue on Windows.

@krassowski
Copy link
Member Author

Also why do we use the BufferedIOBase in the first place? Could it be causing the weird performance issue for Window users? Just bouncing ideas I had when looking at the code....

@bollwyvl
Copy link
Collaborator

bollwyvl commented Jan 24, 2021 via email

@krassowski krassowski deleted the fix-truncated-stream branch July 25, 2021 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Large websocket responses get truncated and cannot be parsed
2 participants