Skip to content

Process: Avoid deadlock when standard streams are not redirected to files #4173

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

Closed
jonilam opened this issue Dec 17, 2021 · 6 comments
Closed
Assignees
Labels
Milestone

Comments

@jonilam
Copy link

jonilam commented Dec 17, 2021

One of our test cases started to hang after switching from Run And Return Rc And Output-keyword to a Run Process-keyword. We debugged the issue and noticed that one of the threads of our test application was stuck in fflush(). Issue is caused by the usage of Popen.wait() + read in Process-library. There is a note about this in a python subprocess-documentation:

"This will deadlock when using stdout=PIPE or stderr=PIPE and the child process generates enough output to a pipe such that it blocks waiting for the OS pipe buffer to accept more data. Use Popen.communicate() when using pipes to avoid that."
https://docs.python.org/3/library/subprocess.html#subprocess.Popen.wait

As stated by the note above, this happens only when pipes are used, and can be avoided by writing stdout/stderr to a file. I verified that indeed using Popen.communicate() instead of Popen.wait() fixes the problem.

Noticed the issue with RF 3.1.1 but tested that the issue is still there in version 4.1.3.

@pekkaklarck
Copy link
Member

pekkaklarck commented Dec 17, 2021

I don't remember why exactly we use wait and not communicate. We possibly could change that if the user hasn't provided stdout and stderr. That's the default so it could avoid hanging also in other cases.

Did you test does redirecting stdout/err to a file fix the problem? If it does, that's a good workaround.

@jonilam
Copy link
Author

jonilam commented Dec 20, 2021

Yes, we are currently using stdout/err redirecting to a file as a workaround and it is working without issues.

@pekkaklarck
Copy link
Member

Good to know that redirecting to files works. It would still be good to investigate could we use communicate when stdout and stderr are PIPEs (which is the default) to avoid deadlocking in cases like this. A problem is that although we call wait(), we only do that after elsewhere calling poll() to verify that the process has stopped and my assumption is that it hangs already there. It's thus possible we needed to restructure the code a bit more. Good thing is that we have lot of tests so even bigger changes can be done safely.

Unfortunately I'm super busy and don't have time to look at this in the near future. I leave this issue open in case someone else is interested to look at it.

@pekkaklarck pekkaklarck changed the title Run process-keyword hangs if process generates enough output to a pipe Process: Use Popen.communicate(), not Popen.wait() when stdout or stderr is a PIPE to avoid deadlocks Dec 22, 2021
@pekkaklarck pekkaklarck changed the title Process: Use Popen.communicate(), not Popen.wait() when stdout or stderr is a PIPE to avoid deadlocks Process: Use Popen.communicate(), not Popen.wait(), when stdout or stderr is PIPE to avoid deadlocks Dec 22, 2021
@pekkaklarck pekkaklarck added this to the v7.3 milestone Jan 2, 2025
@pekkaklarck
Copy link
Member

PR #5302 implements this.

@franzhaas
Copy link
Contributor

this is also related to this issue.: #5345

@pekkaklarck pekkaklarck added acknowledge To be acknowledged in release notes effort: small and removed enhancement help wanted Extra help appreciated labels Mar 7, 2025
@pekkaklarck pekkaklarck changed the title Process: Use Popen.communicate(), not Popen.wait(), when stdout or stderr is PIPE to avoid deadlocks Process: Avoid deadlock when standard streams are not redirected to files Mar 10, 2025
@pekkaklarck
Copy link
Member

I fixed this in f2f2f99. The fix is based on PR #5302 by @franzhaas, but contains some cleanup, doc updates, and fix for handling closed stdout/stderr PIPEs. Big thanks for the PR!

pekkaklarck added a commit that referenced this issue Mar 21, 2025
Docs were slightly outdated after handling outputs was enhanced
in #4173.
@pekkaklarck pekkaklarck self-assigned this May 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants