Revisiting the fix for nested exception handling

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Revisiting the fix for nested exception handling

Nicolas Cellier
Hi all,
In order to purge the inbox, I've been in the process of reviewing
brainfuck code (a very slow process). While at it, I decided to
revisit the solution for nested exception handlers (see
testHandlerFromAction).

There are two similar and obsolete fixes in the inbox (due to
ContextPart->Context transition), Kernel-nice.857 and Kernel-fbs.870
(backported from Cuis) that I moved to treated inbox for that reason.
https://source.squeak.org/treated/Kernel-nice.857.diff
https://source.squeak.org/treated/Kernel-fbs.870.diff

Alas, those two solutions break the very peculiar expectations of
testHandlerReentrancy. This is a pity, because to my knowledge, those
expectations are essential for Tweak and Croquet.

There was a previous attempt from Andreas which was more a simple hack
in handleSignal:, that is to disable the handler by setting the temp
variable handlerActive in on:do: to false (self tempAt: 3 put: false).
I've generalized this solution.
https://source.squeak.org/treated/Kernel-ar.540.diff

However, there are several reasons why this does not work: the first
one, objected by Julian Fitzell here
http://lists.squeakfoundation.org/pipermail/squeak-dev/2011-January/156453.html
is that the handler must be able to handle a secondary exception
raised by the defaultAction. We can find similar expectations with
other exception handling, like resignalAs (I will publish another test
soon).

However, there is some simple strategy: before resuming the exception,
scan the context stack a second time so as to rearm the exception
handlers that we disabled during handleSignal:.

The major drawback, is that I have to spread a few self
reactivateHandlers here and there, virtually before every send of
#resumedUnchecked:.
The second drawback is that scanning the stack a second time is not
the most economical solution, but I don't care too much, we have to
make it right first.

If you find the courage to review, or more easily just to test it,
find it in the inbox...
Oups, I accidentally published in trunk...
Well the review will be forced, if you don't like it, admin please
remove or revert.
Sorry.

Reply | Threaded
Open this post in threaded view
|

Re: Revisiting the fix for nested exception handling

marcel.taeumel
Hi Nicolas,

thanks for looking into this.

4 senders and 3 implementors of #reactivateHandlers isn't too bad. However, I miss an implementation of #deactivateHandlers. I suppose it is that new line in #handleSignal:? Would you mind adding that for readability? :-)

Best,
Marcel

Am 11.04.2021 19:37:11 schrieb Nicolas Cellier <[hidden email]>:

Hi all,
In order to purge the inbox, I've been in the process of reviewing
brainfuck code (a very slow process). While at it, I decided to
revisit the solution for nested exception handlers (see
testHandlerFromAction).

There are two similar and obsolete fixes in the inbox (due to
ContextPart->Context transition), Kernel-nice.857 and Kernel-fbs.870
(backported from Cuis) that I moved to treated inbox for that reason.
https://source.squeak.org/treated/Kernel-nice.857.diff
https://source.squeak.org/treated/Kernel-fbs.870.diff

Alas, those two solutions break the very peculiar expectations of
testHandlerReentrancy. This is a pity, because to my knowledge, those
expectations are essential for Tweak and Croquet.

There was a previous attempt from Andreas which was more a simple hack
in handleSignal:, that is to disable the handler by setting the temp
variable handlerActive in on:do: to false (self tempAt: 3 put: false).
I've generalized this solution.
https://source.squeak.org/treated/Kernel-ar.540.diff

However, there are several reasons why this does not work: the first
one, objected by Julian Fitzell here
http://lists.squeakfoundation.org/pipermail/squeak-dev/2011-January/156453.html
is that the handler must be able to handle a secondary exception
raised by the defaultAction. We can find similar expectations with
other exception handling, like resignalAs (I will publish another test
soon).

However, there is some simple strategy: before resuming the exception,
scan the context stack a second time so as to rearm the exception
handlers that we disabled during handleSignal:.

The major drawback, is that I have to spread a few self
reactivateHandlers here and there, virtually before every send of
#resumedUnchecked:.
The second drawback is that scanning the stack a second time is not
the most economical solution, but I don't care too much, we have to
make it right first.

If you find the courage to review, or more easily just to test it,
find it in the inbox...
Oups, I accidentally published in trunk...
Well the review will be forced, if you don't like it, admin please
remove or revert.
Sorry.



Reply | Threaded
Open this post in threaded view
|

Re: Revisiting the fix for nested exception handling

Nicolas Cellier
Hi Marcel,
yes, we can start naming the mysterious (tempAt:) and (tempAt:put:)
operations with proper methods.
That's what Andres Valloud did in Cuis.
I did the bare minimum changes that could possibly work.
That would be #deactivateHandler, because handleSignal: will only
deactivate one at a time.
The question about senders is whether there's one missing or not...
We have to review each and every possible handling action, and ask
again if ever we had a new action (not everyday's job, OK).

Le lun. 12 avr. 2021 à 10:59, Marcel Taeumel <[hidden email]> a écrit :

>
> Hi Nicolas,
>
> thanks for looking into this.
>
> 4 senders and 3 implementors of #reactivateHandlers isn't too bad. However, I miss an implementation of #deactivateHandlers. I suppose it is that new line in #handleSignal:? Would you mind adding that for readability? :-)
>
> Best,
> Marcel
>
> Am 11.04.2021 19:37:11 schrieb Nicolas Cellier <[hidden email]>:
>
> Hi all,
> In order to purge the inbox, I've been in the process of reviewing
> brainfuck code (a very slow process). While at it, I decided to
> revisit the solution for nested exception handlers (see
> testHandlerFromAction).
>
> There are two similar and obsolete fixes in the inbox (due to
> ContextPart->Context transition), Kernel-nice.857 and Kernel-fbs.870
> (backported from Cuis) that I moved to treated inbox for that reason.
> https://source.squeak.org/treated/Kernel-nice.857.diff
> https://source.squeak.org/treated/Kernel-fbs.870.diff
>
> Alas, those two solutions break the very peculiar expectations of
> testHandlerReentrancy. This is a pity, because to my knowledge, those
> expectations are essential for Tweak and Croquet.
>
> There was a previous attempt from Andreas which was more a simple hack
> in handleSignal:, that is to disable the handler by setting the temp
> variable handlerActive in on:do: to false (self tempAt: 3 put: false).
> I've generalized this solution.
> https://source.squeak.org/treated/Kernel-ar.540.diff
>
> However, there are several reasons why this does not work: the first
> one, objected by Julian Fitzell here
> http://lists.squeakfoundation.org/pipermail/squeak-dev/2011-January/156453.html
> is that the handler must be able to handle a secondary exception
> raised by the defaultAction. We can find similar expectations with
> other exception handling, like resignalAs (I will publish another test
> soon).
>
> However, there is some simple strategy: before resuming the exception,
> scan the context stack a second time so as to rearm the exception
> handlers that we disabled during handleSignal:.
>
> The major drawback, is that I have to spread a few self
> reactivateHandlers here and there, virtually before every send of
> #resumedUnchecked:.
> The second drawback is that scanning the stack a second time is not
> the most economical solution, but I don't care too much, we have to
> make it right first.
>
> If you find the courage to review, or more easily just to test it,
> find it in the inbox...
> Oups, I accidentally published in trunk...
> Well the review will be forced, if you don't like it, admin please
> remove or revert.
> Sorry.
>
>

Reply | Threaded
Open this post in threaded view
|

Code reviews (was: Revisiting the fix for nested exception handling)

Christoph Thiede
In reply to this post by Nicolas Cellier

Hi Nicolas,


In order to purge the inbox, I've been in the process of reviewing brainfuck code (a very slow process). While at it, I decided to revisit the solution for nested exception handlers (see testHandlerFromAction).


I appreciate both of these very much, thank you for your efforts! (Regular) code review and integration is an important process, in particular for a bipartite community like us. I'm aware of the fact that personally, I am only piling up new proposals that need a review but almost never have helped to carry off/review existing ones. If there is anything I (or other interested people) could do to support you in cleaning up the inbox - that goes beyond the administrative power to press the merge button (a privilege which I consider myself not mature for at the moment) -, please let me know!

Best,
Christoph


Von: Squeak-dev <[hidden email]> im Auftrag von Nicolas Cellier <[hidden email]>
Gesendet: Sonntag, 11. April 2021 19:36 Uhr
An: The general-purpose Squeak developers list
Betreff: [squeak-dev] Revisiting the fix for nested exception handling
 
Hi all,
In order to purge the inbox, I've been in the process of reviewing
brainfuck code (a very slow process). While at it, I decided to
revisit the solution for nested exception handlers (see
testHandlerFromAction).

There are two similar and obsolete fixes in the inbox (due to
ContextPart->Context transition), Kernel-nice.857 and Kernel-fbs.870
(backported from Cuis) that I moved to treated inbox for that reason.
https://source.squeak.org/treated/Kernel-nice.857.diff
https://source.squeak.org/treated/Kernel-fbs.870.diff

Alas, those two solutions break the very peculiar expectations of
testHandlerReentrancy. This is a pity, because to my knowledge, those
expectations are essential for Tweak and Croquet.

There was a previous attempt from Andreas which was more a simple hack
in handleSignal:, that is to disable the handler by setting the temp
variable handlerActive in on:do: to false (self tempAt: 3 put: false).
I've generalized this solution.
https://source.squeak.org/treated/Kernel-ar.540.diff

However, there are several reasons why this does not work: the first
one, objected by Julian Fitzell here
http://lists.squeakfoundation.org/pipermail/squeak-dev/2011-January/156453.html
is that the handler must be able to handle a secondary exception
raised by the defaultAction. We can find similar expectations with
other exception handling, like resignalAs (I will publish another test
soon).

However, there is some simple strategy: before resuming the exception,
scan the context stack a second time so as to rearm the exception
handlers that we disabled during handleSignal:.

The major drawback, is that I have to spread a few self
reactivateHandlers here and there, virtually before every send of
#resumedUnchecked:.
The second drawback is that scanning the stack a second time is not
the most economical solution, but I don't care too much, we have to
make it right first.

If you find the courage to review, or more easily just to test it,
find it in the inbox...
Oups, I accidentally published in trunk...
Well the review will be forced, if you don't like it, admin please
remove or revert.
Sorry.



Carpe Squeak!