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

changes p.stdout.read to communicate() #50

Closed
wants to merge 3 commits into from

Conversation

samgdotson
Copy link
Contributor

This PR closes #49 by updating fileutils.run() with subprocess.Popen.communicate() rather than Popen.stdout.read() (and similar).

Currently, watts runs external programs by creating a child process with the subprocess package and read/writes the output to the terminal (and a temporary log file) using subprocess.Popen.stdout.read() and .write(). According to the subprocess docs, this can result in a deadlock. The safer approach is to use communicate() instead.

As explained in #49 this issue was discovered because watts deadlocked anytime it ran the abce code. The fix in this PR seems to preserve the intended usage of watts after some ad hoc test runs with abce. However, there are still some issues with abce itself, and since abce is the only code I have access to (within the watts ecosystem), I haven't been able to do more exhaustive tests. Further, I'm still unsure of the following items:

  • does the communicate block need to be wrapped in while loop?
  • what should the default wait time be? If any at all?

@nstauff nstauff closed this Jul 11, 2022
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.

Using communicate() rather than read() in fileutils.run()
2 participants