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 EPIPE error when hitting quit #225

Merged
merged 3 commits into from
Jan 17, 2019
Merged

Fix EPIPE error when hitting quit #225

merged 3 commits into from
Jan 17, 2019

Conversation

elazzabi
Copy link
Member

This error is raised when trying to write to a closed stream. Node doesn't get rid of those extra messages and raises an error as per nodejs/node#947

From the same issue, it's supposed to be fixed in 8 and up. I tested in node 8 and 10 and I get the error.

This checks if the error code is EPIPE and return.

fixes #175

@elazzabi
Copy link
Member Author

Steps to test:

  1. pull PR
  2. run npm link
  3. run vip app list
  4. hit q
    It should quit gracefully

@joshbetz
Copy link
Contributor

Do we need to unpipe the pager when the main process exits?

@elazzabi
Copy link
Member Author

@joshbetz not sure what do you mean

@joshbetz
Copy link
Contributor

Sorry. If the issue is that we're trying to write to a closed stream, can we avoid writing to a closed stream by calling .unpipe() when the stream is closed?

@elazzabi
Copy link
Member Author

I'm still not that familiar with how streams work in the code base, but I've added .unpipe() to places where I think it belongs (https://github.com/Automattic/vip/blob/master/src/lib/cli/pager.js#L29 and https://github.com/Automattic/vip/blob/master/src/lib/cli/pager.js#L35) but it looks like it doesn't resolve the problem

@joshbetz
Copy link
Contributor

joshbetz commented Dec 19, 2018

I think the problem is here:

https://github.com/Automattic/vip/blob/master/src/lib/cli/command.js#L326-L328

When this was originally added, I noted (#139):

This is a good start, but it would be better if we could page over
results and write 10 at a time. It would also be nice if that was
pausable (like stream.pause()) so we could lazy load results (or never
load results) that aren't going to be on screen anyway.

Ideally that would be a function that returns a stream of a CLI table and when we call stream.read() (which is what happens internally when you pipe a read stream to a write stream), it queries the next page of results from the API and formats the table.

Then, instead of manually calling pager.write() and pager.end(), we'd just pipe the results to the pager.

@elazzabi
Copy link
Member Author

...it queries the next page of results from the API and formats the table.

I wonder if we are optimizing for ourselves instead of our users here. I think the average user don't have a list of more than few apps and we can afford getting them all in one request, don't you think so?

nickdaugherty
nickdaugherty previously approved these changes Jan 16, 2019
@elazzabi elazzabi merged commit 00e25e3 into master Jan 17, 2019
@elazzabi elazzabi deleted the fix/epipe-error branch January 17, 2019 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quitting the display of an app list shows an error
3 participants