Gitpython: If gevent is used, it causes any error to be a gevent.FileObjectClosed(IOError)

[aethanol]

gevent.monkey.patch_all() patches stderr to be gevent._fileobjectcommon._ClosedIO object, which raises its own FileObjectClosed(IOError).

class _ClosedIO(object):
    ...
    def __getattr__(self, name):
        if name == 'name':
            # We didn't set it in __init__ because there wasn't one
            raise AttributeError
        raise FileObjectClosed

GitPython only catches ValueError in read_all_from_possibly_closed_stream

def read_all_from_possibly_closed_stream(stream: Union[IO[bytes], None]) -> bytes:
    if stream:
        try:
            return stderr_b + force_bytes(stream.read())
        except ValueError:
            return stderr_b or b''
    else:
        return stderr_b or b''

[aethanol]

This could be argued as a bug in gevent, since they are changing the expected behavior.

[Byron]

Thanks for reporting, and I agree with the assessment.

I'd be very interested in hearing which action GitPython is supposed to take. Options I see would be…

1. depend on gevent and catch their exception type as well. Ideally that's conditional, maybe based on the presence of gevent in the packages available for import.
2. trying to undo the monkey-patch proactively. That would probably break gevent itself

Option 1 seems like a possible avenue. I could imagine something like this.

try:
    import gevent.SpecialFSException
    IOException = SpecialFSException
except ImportError:
    IOException = ValueError

Then this conditional exception type could be used

def read_all_from_possibly_closed_stream(stream: Union[IO[bytes], None]) -> bytes:
    if stream:
        try:
            return stderr_b + force_bytes(stream.read())
        except IOException:
            return stderr_b or b''
    else:
        return stderr_b or b''

This would only work if gevent always patches the python runtime.

[jbirch-atlassian]

The documentation for the basic io implementations has indications that callers should expect, at the very least, OSError on some operations.

Given that gevent's FileObjectClosed inherits from IOError, which is an OSError, I can see the argument that a particularly robust read_all_from_possibly_closed_stream would except both (ValueError, OSError). Truthfully, the standard library documentation could be more prescriptive here, and gevent is bending the rules a little by having a custom class that has no ties to any IO bits that exists entirely to drop objects early — always makes for an awkward attribution of emergent behaviour.

[Byron]

Just catching (ValueError, OSError) would definitely be a much preferred solution! Thanks so much.
@aethano or @jbirch-atlassian l Would you be able to submit a PR with this fix?