Skip to content

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

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
aethanol opened this issue Sep 7, 2021 · 4 comments · Fixed by #1464
Closed

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

aethanol opened this issue Sep 7, 2021 · 4 comments · Fixed by #1464

Comments

@aethanol
Copy link

aethanol commented Sep 7, 2021

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
Copy link
Author

aethanol commented Sep 7, 2021

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

@Byron
Copy link
Member

Byron commented Sep 7, 2021

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
Copy link

jbirch-atlassian commented Sep 8, 2021

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
Copy link
Member

Byron commented Sep 8, 2021

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants