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

Close files on SpawnProcess#stop #150

Merged
merged 4 commits into from
May 9, 2013

Conversation

JonRowe
Copy link
Contributor

@JonRowe JonRowe commented May 8, 2013

Alternate implementation to #149

SpawnProcess creates files to use for stdout and stderr which are opened but never closed, this leaks file descriptors that eventually cause larger test suites to fail on some systems (I encountered this with rspec-core ).

This implementation closes the tmp files during the SpawnProcess#stop(reader) thus cleaning up the files and preventing the issue, however this causes an issue if steps attempt to access the files after the process has finished, to handle that eventuality the output of reading the streams is captured and used in the event of an IOError.

@jarl-dk
Copy link
Member

jarl-dk commented May 8, 2013

That looks good. I guess the rescue is to handle the case where the process is done (@process is nil) and files are closed. I'd rather see that this particular situation is handled explicitly with an if condition and remove the rescue clause and leave exceptions to exceptional situations...

@JonRowe
Copy link
Contributor Author

JonRowe commented May 8, 2013

I did try using an if even though I have an irrational hatred of them, but I couldn't make it a nicer solution, mainly because you can't actually check that files are closed or open other than to try reading/writing to them. I might try again.

@jarl-dk
Copy link
Member

jarl-dk commented May 8, 2013

I would even go so far as to set @out = nil (the same for @err) in #stop. But before you close (and delete) them in #stop you must ensure to read all content and put it in the _cache equivalent.

@JonRowe
Copy link
Contributor Author

JonRowe commented May 8, 2013

You might prefer this version...

@jarl-dk
Copy link
Member

jarl-dk commented May 8, 2013

How about this? https://github.com/cucumber/aruba/tree/close_tmp_files_on_stop its this PR with one additional commit (1fd252e)

@JonRowe
Copy link
Contributor Author

JonRowe commented May 8, 2013

Works for me.

jarl-dk added a commit that referenced this pull request May 9, 2013
@jarl-dk jarl-dk merged commit 4051be8 into cucumber:master May 9, 2013
@JonRowe
Copy link
Contributor Author

JonRowe commented May 9, 2013

Thanks :)

@jarl-dk
Copy link
Member

jarl-dk commented May 9, 2013

Thanks again for your involvement and contribution.

lzap pushed a commit to lzap/hub that referenced this pull request Nov 11, 2013
mislav added a commit to mislav/hub that referenced this pull request Dec 18, 2013
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.

2 participants