Opened 7 years ago

Closed 7 years ago

#15488 closed defect (fixed)

Fix misplaced sig_on() inside try

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-5.13
Component: c_lib Keywords:
Cc: Merged in: sage-5.13.rc0
Authors: Jeroen Demeyer Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

Many of these are my own fault, but

try:
    sig_on()
    ...
finally:
    sig_off()

should become

sig_on()
try:
    ...
finally:
    sig_off()

Attachments (1)

15488_try_sig_on.patch (17.1 KB) - added by jdemeyer 7 years ago.

Download all attachments as: .zip

Change History (13)

Changed 7 years ago by jdemeyer

comment:1 Changed 7 years ago by jdemeyer

  • Status changed from new to needs_review

comment:2 follow-up: Changed 7 years ago by ncohen

  • Status changed from needs_review to needs_info

Hellooooooo Jeroen !

I have two questions about your patch:

  • Why do you create two layers of try/catch in ppl.pyx around rel.thisptr = new_relation_with(self.thisptr[0], g.thisptr[0])
  • Why do you replace ValueError by BaseException ?

Thaaaaaaaaaaaaaaaaaanks !

Nathann

comment:3 in reply to: ↑ 2 ; follow-up: Changed 7 years ago by jdemeyer

Replying to ncohen:

  • Why do you create two layers of try/catch?

Because there is no other way to achieve the same result with only one try/finally ;-)

  • Why do you replace ValueError by BaseException ?

Because the only purpose of that except branch is to put the object in a consistent state before re-raising the exception. I guess the "putting the object in a consistent state" part should also be done when other exceptions happen.

comment:4 Changed 7 years ago by jdemeyer

  • Status changed from needs_info to needs_review

comment:5 follow-up: Changed 7 years ago by ncohen

Yooooooooooooo !!

Thanks for your answer, but I still need to know more about the first question. In which case does your code behave differently from the former one ? Why didn't you just move the sig_off out ?

Nathann

comment:6 in reply to: ↑ 5 ; follow-up: Changed 7 years ago by jdemeyer

Replying to ncohen:

In which case does your code behave differently from the former one ?

In the case that sig_on() raises an exception. Then sig_off() should not be called but setting rel.thisptr to a sensible value should happen.

comment:7 in reply to: ↑ 3 ; follow-up: Changed 7 years ago by pbruin

Replying to jdemeyer:

  • Why do you replace ValueError by BaseException ?

Because the only purpose of that except branch is to put the object in a consistent state before re-raising the exception. I guess the "putting the object in a consistent state" part should also be done when other exceptions happen.

And except BaseException: is equivalent to except:, since all exceptions derive from BaseException, don't they? The second form looks slightly clearer to me.

comment:8 in reply to: ↑ 6 Changed 7 years ago by pbruin

Replying to jdemeyer:

Replying to ncohen:

In which case does your code behave differently from the former one ?

In the case that sig_on() raises an exception. Then sig_off() should not be called but setting rel.thisptr to a sensible value should happen.

It looks as if another way to solve this is to check if self.thisptr != NULL in ___dealloc__(). This is how a similar problem was fixed in #12888. (Note: Cython initialises attributes to NULL.)

comment:9 in reply to: ↑ 7 Changed 7 years ago by jdemeyer

Replying to pbruin:

And except BaseException: is equivalent to except:, since all exceptions derive from BaseException, don't they? The second form looks slightly clearer to me.

Explicit is better than implicit (see The Zen of Python). I prefer except BaseException: because it's explicit that I really want to catch all exceptions.

In almost all cases where I saw except:, it was by mistake. In fact, as release manager, I simply rejected any patches containing except:.

comment:10 follow-up: Changed 7 years ago by vbraun

  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review

lgtm

Replying to pbruin:

It looks as if another way to solve this is to check if self.thisptr != NULL in ___dealloc__().

Yes, there is more than one way to solve this.

comment:11 in reply to: ↑ 10 Changed 7 years ago by pbruin

Replying to vbraun:

Yes, there is more than one way to solve this.

More wisdom from The Zen of Python (see above):

There should be one-- and preferably only one --obvious way to do it.
Although that way may not be obvious at first unless you're Dutch.

Not meant as an objection to this patch or to the positive review!

comment:12 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.13.rc0
  • Resolution set to fixed
  • Status changed from positive_review to closed

Thanks for the quick review!

Note: See TracTickets for help on using tickets.