-
Notifications
You must be signed in to change notification settings - Fork 93
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
subprocpool: handle long STDOUT/ERR from commands #2876
subprocpool: handle long STDOUT/ERR from commands #2876
Conversation
8c44fc9
to
1611241
Compare
Code looks good, Travis happy too. +1 ! 🎉 digress mode: On If while True:
res = os.read(fileno, 65536)
if not res:
break
data += res could simply be while os.read(fileno, 65536) as cm:
data += cm.data Which I think is simpler and goes more along with the Pythonic way... In Java I would write that in a single line, but that's not allowed in Python. Interestingly the only different way I found to work was with import os
from functools import partial
license = os.path.join(os.path.dirname(__file__), 'COPYING')
data = ''
with open(license) as l:
fileno = l.fileno()
for res in iter(partial(os.read, fileno, 1000), ''):
data += res
print(data) But looks like Python 3.8 might solve it with PEP-572, where we will have something like:
digress mode: Off |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
(So does this supersede #2870?) |
This is the full fix. #2870 is still useful in a way - it breaks up the very long submit commands into smaller chunks - so more jobs can be submitted in parallel. |
Hi @kinow (And ditto for Perl. I really miss the the |
(I'll push up a change with a few more lines of extra comments to explain this in the logic.) |
1611241
to
afcaa5b
Compare
(Branch re-based. More comments added to explain the usage of |
Oh, I meant that it would be good if there was some sort of cm around that. I think |
(Should now be good to go!) |
Still looking good to me, thanks for the extra docs! Will leave it up to you to merge or check with @hjoliver if you'd like. 👍 |
Quick thought on this one, the |
afcaa5b
to
d3cb7a3
Compare
(Branch re-based again. Now with logic to bypass the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-approving.
Maybe just comment in the new integration test, that it may fail on systems without select.poll()
?
(Patch coverage 81%; but the detail shows that is good enough I think 😁 ) |
Ensure that pipes with lots of output do not block the command.
If OS does not support `select.poll`.
d3cb7a3
to
0c28766
Compare
(Rebased + hopefully the final tweak - now include logic to skip the new tests on OS without |
Had had a look on the |
Ditto. 👍 |
Ensure that pipes with lots of output do not block the command.
Fully fix #2857.