Repository navigation
If gevent is used, it causes any error to be a gevent.FileObjectClosed(IOError) #1336
Description
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''
Activity
aethanol commentedon Sep 7, 2021
This could be argued as a bug in gevent, since they are changing the expected behavior.
Byron commentedon 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…
geventand catch their exception type as well. Ideally that's conditional, maybe based on the presence ofgeventin the packages available for import.geventitselfOption 1 seems like a possible avenue. I could imagine something like this.
Then this conditional exception type could be used
This would only work if
geventalways patches the python runtime.jbirch-atlassian commentedon Sep 8, 2021
The documentation for the basic io implementations has indications that callers should expect, at the very least,
OSErroron some operations.Given that gevent's
FileObjectClosedinherits fromIOError, which is anOSError, I can see the argument that a particularly robustread_all_from_possibly_closed_streamwould 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 commentedon 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?