Please review: PR # 65

[Shashank Katlaparthi]

Hi,

Could someone take a look at https://github.com/bup/bup/pull/65

The details are in the commit message.

It looks like the CI checks are failing due on issues unrelated to the PR (quick glance)

Cheers,

Shashank

[Rob Browning]

"'Shashank Katlaparthi' via bup-list" <bup-...@googlegroups.com>
writes:

> Could someone take a look at https://github.com/bup/bup/pull/65

Hmm, could you elaborate on the original problem a bit, i.e. assuming
there was a crash, what was happening at the time, and what did the
crash look like? I'd like to understand the original situation a bit
better first.

(And if it turns out that the change suggested is the one we'll want, I
suspect it may need some adjustment (as I think you mentioned in a
comment), i.e. I think InterruptedError may only be available in python
3, and also, the patch itself appears to refer to KeyboardInterrupt
rather than InterruptedError.)

Thanks
--
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4

[Shashank Katlaparthi]

Hi Rob,

So here's the problem - 

03:52:16   File "/usr/lib/bup/bup/git.py", line 634, in _raw_write
03:52:16     f.write(oneblob)
03:52:16 bup.git.GitError: [Errno 28] No space left on device
03:52:16 Exception IOError: (28, 'No space left on device') in <bound method PackWriter.__del__ of <bup.git.PackWriter instance at 0x7effc547bbe0>> ignored

as you can see in the traceback above https://github.com/bup/bup/blob/master/lib/bup/git.py#L788-L791 the condition here is actually handling a legit I/O error that we should've been raised and terminated the operation.

From the comments in the code it appears we are catching the exception of I/O Errors to handle keyboard interrupts and maybe other needed system calls, which is great, but out-of-disk-space errors do come under "I/O Errors" as well. To explicitly handle the errors we intend to catch we need to switch to https://docs.python.org/3/library/exceptions.html#InterruptedError BUT since InterruptedError isn't available in python 2 -- I made the change in the code to use https://docs.python.org/2.7/library/exceptions.html#exceptions.KeyboardInterrupt which is supported by both Py 2 and 3.

Although, I'd suggest using the `InterruptedError` as it handles a wider set of I/O exceptions than `KeyboardInterrupt` when we are at a point when we are no longer using py 2 or adjust the current code to detect python version to use appropriately use the suitable exception.

Thanks,

Shashank

[Shashank Katlaparthi]

bump

[Shashank Katlaparthi]

bu(m)p!

[Rob Browning]

Shashank Katlaparthi <skatla...@snapchat.com> writes:

> *03:52:16 File "/usr/lib/bup/bup/git.py", line 634, in _raw_write03:52:16
> f.write(oneblob)03:52:16 bup.git.GitError: [Errno 28] No space left on
> device03:52:16 Exception IOError: (28, 'No space left on device') in <bound

> method PackWriter.__del__ of <bup.git.PackWriter instance at

> 0x7effc547bbe0>> ignored*

> as you can see in the traceback above
> https://github.com/bup/bup/blob/master/lib/bup/git.py#L788-L791 the
> condition here is actually handling a legit I/O error that we should've
> been raised and terminated the operation.

I'm still not sure I understand. That code (now here?)
https://github.com/bup/bup/blob/8840a24b8328c961c620565059a913b03f3af3ab/lib/bup/git.py#L838
just re-raises the IOError as a GitError, which I'd assume eventually
does cause bup to terminate. Are you saying you found that it doesn't?