subprocpool: handle long STDOUT/ERR from commands

[matthewrmshin]

Ensure that pipes with lots of output do not block the command.

Fully fix #2857.

[kinow]

Code looks good, Travis happy too. +1 ! 🎉

digress mode: On

If os.read returned a context manager, or if there was a library for that, maybe that while block

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

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:

while res := os.read(fileno, 1000):
    data += res

digress mode: Off

[hjoliver]

LGTM

[hjoliver]

(So does this supersede #2870?)

[matthewrmshin]

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.

[matthewrmshin]

Hi @kinow os.read is a low level function that wraps the POSIX read system call. I don't think it returns a Pythonic context manager. The system call is used here (i.e. os.read(handle.fileno, ...) instead of handle.read(...) to make sure that we don't get any funny buffering while we read the pipes. The code looks clunky but is actually safest.

(And ditto for Perl. I really miss the the while assignment statement when I switched from Perl to Python - it allows a person inspecting the code to quick know what is driving the exit condition of a while loop - code is much more readable and much safer.)

[matthewrmshin]

(I'll push up a change with a few more lines of extra comments to explain this in the logic.)

[matthewrmshin]

(Branch re-based. More comments added to explain the usage of os.read instead of file.read.)

[kinow]

Hi @kinow os.read is a low level function that wraps the POSIX read system call. I don't think it returns a Pythonic context manager.

Oh, I meant that it would be good if there was some sort of cm around that. I think open is a context manager for a low level API too (with the buffer detail that I didn't know! TIL! Thanks 😀 ).

[matthewrmshin]

(Should now be good to go!)

[kinow]

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

[oliver-sanders]

Quick thought on this one, the select.poll interface isn't quite universal so it might be good to maintain the current mostly-working approach for systems which don't support select.poll. I don't know whether it would be possible to just stick the .poll bit in a try/except block?

See #2591, #2689

[matthewrmshin]

(Branch re-based again. Now with logic to bypass the select.poll code if it is not supported by the OS.)

[hjoliver]

Re-approving.

Maybe just comment in the new integration test, that it may fail on systems without select.poll()?

[hjoliver]

(Patch coverage 81%; but the detail shows that is good enough I think 😁 )

[matthewrmshin]

(Rebased + hopefully the final tweak - now include logic to skip the new tests on OS without select.poll.)

[kinow]

Had had a look on the select changes over the weekend. Just had another look, everything looking good. 👍 keeping approval

[hjoliver]

Ditto. 👍