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

Cleanup SpawnProcess files after use #149

Closed
wants to merge 3 commits into from

Conversation

JonRowe
Copy link
Contributor

@JonRowe JonRowe commented May 6, 2013

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).

So before we clear processess we clean them up by closing their io, for completness I also added this to InProcess.

@mattwynne
Copy link
Member

Thanks for this. Did you consider putting the logic into, say Aruba::Api#terminate_processes! and calling that from the hook instead?

@JonRowe
Copy link
Contributor Author

JonRowe commented May 6, 2013

Yes, my reading of the logic was that terminate_processes! is called then stop_processess! so initially I put this cleanup at the end of stop, unfortunately the outputs may be read after the process has stopped in order to perform assertions against the output, this results in an error. So instead I took the approach to close them only at the last possible moment. It would be possible to refactor this to a cleanup_processess! or some such that closes all the IO and then clears the process hash if you would prefer.

@jarl-dk
Copy link
Member

jarl-dk commented May 8, 2013

@JonRowe : Thank you for the involvement. I think however that the closing of the IO should take place at spawn_process#stop(reader) and in_process#stop(reader) and before closing them the content should be captured in a member variable so that #stdout and #stderr uses these values in case the @process is nil (equivalent to the files are closed).

Would it be possible to make a test that demonstrates the increasing number of open files as scenarios are executed, and that this PR solves this issue?

I suggest you create an issue explaining the problem to which this PR is one among many possible solutions.

@JonRowe
Copy link
Contributor Author

JonRowe commented May 8, 2013

@jarl-dk sure, run rspec-core on 1.8.7 with and without this patch

@JonRowe
Copy link
Contributor Author

JonRowe commented May 8, 2013

Sorry, that was snippy and unconstructive, the problem is that this is a heisenbug caused by individual system, file system limits which causes erratic behaviour in large test suites using aruba, pretty much the only way to test this would be to hook into lsof and check that the file descriptors attached to the process are the same before and after a scenario, but to do that you'd need to run aruba from aruba, is that too meta?

@jarl-dk
Copy link
Member

jarl-dk commented May 8, 2013

@JonRowe : could you ellaborate a bit on how to reproduce (without the fix). What do you mean run rspec-core?

@JonRowe
Copy link
Contributor Author

JonRowe commented May 8, 2013

That's what I meant about not being helpful, basically its a heisenbug that keeps occurring in the rspec-core cucumber suite (happening more frequently on 1.8.7), on certain machines it will reach the open file descriptor limit during the cucumber run, and from then on it will produce Error::EMFile "too many open files" and falsely fail.

Through use of lsof I tracked this down to Aruba leaking files from SpawnProcess

@jarl-dk
Copy link
Member

jarl-dk commented May 9, 2013

I just cloned rspec-core and ran bundle exec rake cucumber with ruby-1.8.7-p370. No problem here (on Ubuntu 12.10 64bit). What OS are you using? I am just curious now...

@jarl-dk
Copy link
Member

jarl-dk commented May 9, 2013

Superseded by #150

@jarl-dk jarl-dk closed this May 9, 2013
@JonRowe
Copy link
Contributor Author

JonRowe commented May 9, 2013

I'm on OS X, it's an intermittent bug... My open file limit is 2560.

~$ ulimit -a | grep 'open files'
open files                      (-n) 2560

and you can find (with a bit of tinkering) open files with

ps aux | grep ruby | awk '{print $2}' | xargs -n1 -I 'foo' lsof -p 'foo' | sort | awk '{print $2}' | uniq -c

which I based off this article

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.

3 participants