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

Make sure to drain the websocket TextMessage #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

slaskis
Copy link

@slaskis slaskis commented Mar 30, 2022

Fixes the failing test I created in convox/convox#426 and should fix convox/issues#9

@heronrs
Copy link
Contributor

heronrs commented Mar 30, 2022

@slaskis thanks for the PR!
So this should fix piping output through the convox exec command, right?
Is there any simple manual test I can do on my side to validate that?
Just piping a big text file should be enough?

@slaskis
Copy link
Author

slaskis commented Mar 30, 2022

Yep that should be enough. Its a bit racy as it depends on how much was able to pass through the TextMessage io.Reader .Read call but ive seen it fail sort of reliable around 16kb and up.

And I believe it is the same code used for process input output as well :)

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.

convox exec piping seems to drop packets
2 participants