The Trunk: Kernel-nice.1384.mcz

Am 21.04.2021 21:28:26 schrieb Jakob Reschke <[hidden email]>:

I think from a user/client perspective, there are three different problems here:
1. ProgressInitiationException,
2. what to rearm/reactivate after resuming from a signal, a.k.a. the
odd effect that signalling an exception twice from the same context
might now handle them with different exception environments (my
"unacceptable"),
3. different behavior if the debugger is used.

I just stumbled upon all three problems on a single test case in a
matter of hours. ;-)

What is special about ProgressInitiationException is that you are
supposed to install a handler deeper in the stack and it will run some
code that is supposed to be run further up in the stack. For this use
case it does not feel right to deactivate all the handlers in between
because, after all, redirecting the progress should not really be
exception handling; it just establishes a different context for
certain operations, like a DynamicVariable. I would argue that the
code in the "workBlock" of the ProgressInitiationException is supposed
to be run in the same exception environment as the signalling of the
ProgressInitiationException (maybe minus the progress handler itself
unless rearm is used), because why should progress redirection
interfere with other exception handlers?

Maybe this is futile and an error in the design of
ProgressInitiationException. Its implementation just happened to work
with the previous implementation of exceptions in Squeak.

If I read the ANSI standard correctly, it leans more towards your fix:
"If a matching handler is found, the exception action of the handler
is evaluated in the exception environment that was current when the
handler was created [...]" (p. 91, ANSI Smalltalk Standard Draft
v1.9), and in the specification of "on: selector do: action": "Before
evaluating the receiver the current state of the exception environment
is captured as the handler environment. [...] If signaling of an
exception results in evaluation of action the evaluation will occur in
the context of the handler environment." (p. 85, ibid).

Trying to fix ProgressInitiationException by introducing the
reactivateHandlers as you suggested, would also reactivate handlers
that have already fired, wouldn't it?

For example:
1. One context at the top of the stack signals an error.
2. It is handled by an exception handler from the middle of the stack.
3. The handler in 2. runs a lengthy operation that displays progress,
signalling a ProgressInitiationException. Could download a missing
file, for example.
4. Let some more unrelated exception handlers be below that in the
stack. They do not handle the ProgressInitiationException.
5. Even further down the stack, a ProgressInitiationException handler
will handle what was signalled as progress in 3.

If this progress handler in 5. would rearm all handlers, it would also
rearm the handler from 2. that really handles an error situation out
of scope of the current activity (the lengthy recovery operation in
3). So I think 5. must not rearm the handler in 2., but the handlers
in 4. should remain active (special case for
ProgressInitiationException) because otherwise the progress handling
effectively strips away all established exception handling while the
workBlock of the ProgressInitiationException is evaluated. If 5.
eventually resumes the ProgressInitiationException, it should return
to 3., but it should not rearm the handler in 2. since it is still
being evaluated.

In ProgressInitiationException class>>testWith, the ZeroDivide
exception is an example of an exception like in 3. above. Its handler
gets stripped away now. Another example of it is my
Environment>>beCurrentDuring: that is ignored now with this version.

Could you please check whether resuming from 5. to 3. in the example
above would rearm the handler in 2. now? I might think more about
ProgressInitiationException on Friday or the weekend.

Am Mi., 21. Apr. 2021 um 11:49 Uhr schrieb Nicolas Cellier
:

>
> Hi Jakob,
>
> Le mar. 20 avr. 2021 à 19:00, Jakob Reschke a écrit :
> >
> > Hi,
> >
> > Am Di., 20. Apr. 2021 um 09:16 Uhr schrieb Nicolas Cellier
> > :
> > >
> > > Here, you submit another case where you want all the handlers to be rearmed.
> > > I don't think that rearmHandlerDuring: should rearm the other
> > > exceptions upstack.
> > > Maybe you could try (e reactivateHandlers) instead of (e
> > > rearmHandlerDuring: [...]).
> >
> > I also do not think that rearm should rearm all handlers. I shall
> > think a bit more whether reactivateHandlers is the right choice or
> > whether it cannot also reactivate too much.
> >
> > > Let's have a look at what sendNotificationsTo: does
> > >
> > > sendNotificationsTo: aNewBlock
> > > self resume: (
> > > workBlock value: [ :barVal |
> > > aNewBlock value: minVal value: maxVal value: barVal
> > > ]
> > > )
> > >
> > > We recognize the same pattern as resignalAs: and nil>>handleSignal:
> > > resume will reactivateHandlers after the battle, that is after
> > > evaluating aNewBlock...
> > > I suggest this revision:
> > >
> > > sendNotificationsTo: aNewBlock
> > > self reactivateHandlers; resumeUnchecked: (
> > > workBlock value: [ :barVal |
> > > aNewBlock value: minVal value: maxVal value: barVal
> > > ]
> > > )
> > >
> > > I think that it makes rearmHandlerDuring: un-necessary.
> >
> > ...which means that now the use of sendNotificationsTo: cannot be used
> > to deal with just one layer of progress. It will always also apply to
> > multiple nested progress bars. Not a problem for my use case, but I
> > wonder what the general expectations of it are.
> >
>
> Ah, I wondered exactly about that case...
> What we want to achieve is really contradictory here.
> We want to handle the case when a new exception is signalled during
> the handling (during execution of 2nd arg of on:do:)
> #testHandlerFromAction specifies that shallower handlerContext shall
> be bypassed, and that next handler shall be searched deeper than
> active one.
> The solution in https://source.squeak.org/treated/Kernel-nice.857.diff
> was to let handleSignal: be detected as a handlerContext
> (),
> because it already knows the active handlerContext, it's easy to
> continue stack scanning from there...
> Same scheme was later adopted in Cuis (see
> https://source.squeak.org/treated/Kernel-fbs.870.diff).
>
> #testHandlerReentrancy on the other hand specifies that not all the
> shallower handlerContext shall be bypassed, but that some can be
> rearmed and stay active.
> This is challenging to implement with a solution like in Cuis, but I
> think it should be possible...
> The new Squeak solution is much simpler: deactivate when we backtrack
> the stack, reactivate when we resume the exception...
> That also means that it's less fine-grained.
>
> My understanding here is that you have yet another set of expectations:
> - the shallower handlers shall be active during current exception
> handling so as to intercept other kinds of Exception
> Note that this would not work in Cuis
> - except the already fired handlers that have not been rearmed...
> (what you call the stack state)
> It's not only challenging, it's questionable whether this can possibly
> fit with above expectations...
>
> > >
> > > You also raise other points.
> > > Why the following works:
> > > Environment current.
> > > ^ self actualClassIn: Environment current
> > > Environment current sends an extra signal, so signalling twice has a
> > > different effect as signalling once.
> > > You said that it's not acceptable, but I'm not so sure...
> >
> > What I find not acceptable (or, let's rather say questionable) is the
> > hidden side effect. After a signal is fully handled and has returned
> > to the signal sender, I would naïvely expect that the stack (exception
> > environment) is in the same state as before the signal. If custom
> > exceptions or handler blocks implement their own side effects or
> > state, so shall they. But in my opinion the base implementation should
> > not have such confusing side effects on the stack, and on totally
> > unrelated exception handlers in particular.
> >
> > Kind regards,
> > Jakob
> >
>
> Yes, but how does that happen?
> For returning a value to the sender of signal, one has to resume
> (resume:), which will reactivateHandlers.
> There is the case of resumeUnchecked: which leaves this responsibility
> to its sender, and currently, not all senders of resumeUnchecked: will
> reactivateHandlers.
> (I didn't know what to do in SpurImageSegmentLoader for example...)
> Ah, there is also runUntilErrorOrReturnFrom: which might explain some
> differences when debugging...
> but is it really the path of trouble here?
>
>
> >
> >
> > >
> > > Why the debugging takes another path is another weird behavior that we
> > > should inquire...
> > > It ain't gonna be easy though.
> > >
> > > Le mar. 20 avr. 2021 à 00:08, Jakob Reschke a écrit :
> > > >
> > > > Oh by the way, this was helpful to inspect the stack in the incorrect state:
> > > >
> > > > MCMethodDefinition>>
> > > > actualClass
> > > > (thisContext sender selector == #unload) ifTrue: [thisContext
> > > > copyStack inspect].
> > > > ^ self actualClassIn: Environment current
> > > >
> > > > In the Inspector:
> > > > | context |
> > > > context := self nextHandlerContext.
> > > > Array streamContents: [:str |
> > > > [context notNil] whileTrue: [str nextPut: context. context :=
> > > > context nextHandlerContext]].
> > > >
> > > > | context |
> > > > context := self.
> > > > Array streamContents: [:str |
> > > > [context notNil] whileTrue: [str nextPut: context. context :=
> > > > context sender]].
> > > >
> > > > Am Di., 20. Apr. 2021 um 00:02 Uhr schrieb Jakob Reschke
> > > > :
> > > > >
> > > > > Looks like this does no longer work as before:
> > > > >
> > > > > SquotImageStoreTest>>
> > > > > suppressProgressDisplayDuring: aBlock
> > > > > ...
> > > > > aBlock
> > > > > on: ProgressInitiationException do: [:e |
> > > > > ...
> > > > > e rearmHandlerDuring:
> > > > > [[e sendNotificationsTo: [:min :max :current | "silence"]]
> > > > > on: ProgressNotification do: [:notification |
> > > > > notification resume]]
> > > > > ...
> > > > >
> > > > > rearmHandlerDuring: does reactivate this current handler, but not
> > > > > handlers further up the stack.
> > > > > So e sendNotificationsTo: will evaluate the block in the package
> > > > > loader that eventually unloads the method in the test case while all
> > > > > handlers that are between the ProgressInitiationException signal
> > > > > context and this ProgressInitiationException handler context on the
> > > > > stack are deactivated, including the one that sets the correct
> > > > > Environment (and also the source file caching, by the way). Find the
> > > > > annotated stack below for a little more visualization.
> > > > >
> > > > > Previously, the handler contexts that did not fit the raised Exception
> > > > > were not deactivated. Is the ProgressInitiationException redirection
> > > > > concept broken in general now? Note that the ZeroDivide in
> > > > > ProgressInitiationException>>testWith is now no longer caught.
> > > > >
> > > > > --- The redacted stack of my failing test with some ---annotations--->
> > > > > [] in [] in [] in [] in [] in MCPackageLoader>>basicLoad <--- The
> > > > > block under 'Installing ', pkgName displayProgressFrom: ...
> > > > > ProgressInitiationException>>sendNotificationsTo:
> > > > > [] in [] in [] in SquotImageStoreTest>>suppressProgressDisplayDuring:
> > > > > FullBlockClosure(BlockClosure)>>on:do:
> > > > > [] in [] in SquotImageStoreTest>>suppressProgressDisplayDuring:
> > > > > [] in Context>>rearmHandlerDuring:
> > > > > FullBlockClosure(BlockClosure)>>ensure:
> > > > > ---reactivated #1---> Context>>rearmHandlerDuring:
> > > > > ProgressInitiationException(Exception)>>rearmHandlerDuring:
> > > > > [] in SquotImageStoreTest>>suppressProgressDisplayDuring:
> > > > > [] in Context>>handleSignal:
> > > > > FullBlockClosure(BlockClosure)>>ifCurtailed:
> > > > > Context>>handleSignal: <--- #1 sender:
> > > > > SquotImageStoreTest>>suppressProgressDisplayDuring:
> > > > > ...
> > > > > Context>>handleSignal: <--- #2 sender: Environment beCurrentDuring:
> > > > > Context>>handleSignal: <--- sender: CurrentReadOnlySourceFiles
> > > > > class>>cacheDuring:
> > > > > Context>>handleSignal: <--- sender: [] in [] in [] in MCPackageLoader basicLoad
> > > > > ---triggers stack walk---> ProgressInitiationException(Exception)>>signal
> > > > > ProgressInitiationException>>display:at:from:to:during:
> > > > > ...
> > > > > ByteString(String)>>displayProgressFrom:to:during:
> > > > > [] in [] in [] in [] in MCPackageLoader>>basicLoad
> > > > > ---deactivated---> FullBlockClosure(BlockClosure)>>on:do:
> > > > > [] in [] in [] in MCPackageLoader>>basicLoad
> > > > > ---deactivated---> FullBlockClosure(BlockClosure)>>on:do:
> > > > > CurrentReadOnlySourceFiles class>>cacheDuring:
> > > > > ...
> > > > > MCPackageLoader>>basicLoad
> > > > > ...
> > > > > MCPackageLoader>>loadWithNameLike:
> > > > > [] in SquotPackageShadow>>squotMaterializeWith:
> > > > > ---deactivated #2---> FullBlockClosure(BlockClosure)>>on:do:
> > > > > Environment>>beCurrentDuring:
> > > > > SquotPackageShadow>>squotMaterializeWith:
> > > > > ...
> > > > > SquotImageStoreTest>>testApplyPatch
> > > > > SquotImageStoreTest(TestCase)>>performTest
> > > > > [] in SquotImageStoreTest>>performTest
> > > > > ---rearmed handler #1---> FullBlockClosure(BlockClosure)>>on:do:
> > > > > SquotImageStoreTest>>suppressProgressDisplayDuring:
> > > > > ...
> > > > >
> > > > > Am Mo., 19. Apr. 2021 um 22:37 Uhr schrieb Jakob Reschke
> > > > > :
> > > > > >
> > > > > > Hi Nicolas,
> > > > > >
> > > > > > I seem to have a Heisenbug now because of this:
> > > > > > In case you have the Git tools loaded, you should have a SquotImageStoreTest in your image. After loading this version and the follow-up fix versions of Kernel, the test SquotImageStoreTest>>testApplyPatch fails.
> > > > > >
> > > > > > The test fails because an initialize method is not removed from the class to be patched. When the method is about to be MCMethodDefinition>>#unload-ed, it makes a CurrentEnvironment lookup in MCMethodDefiction>>actualClass, and it returns a wrong environment. It seems that the handler to set the dynamic Environment further up in the stack is not activated for this particular lookup. Yet if I debug through the procedure, everything works as it should. If I let the test run on its own, it fails.
> > > > > >
> > > > > > But if I replace in MCMethodDefinition this:
> > > > > >
> > > > > > actualClass
> > > > > > ^ self actualClassIn: Environment current
> > > > > >
> > > > > > by this:
> > > > > >
> > > > > > actualClass
> > > > > > Environment current.
> > > > > > ^ self actualClassIn: Environment current
> > > > > >
> > > > > > Then the test is suddenly green again. I suppose that is not acceptable. ;-)
> > > > > >
> > > > > > It also works with Notification signal instead of the extraneous Environment current, so it seems like the signal handler stack is somehow in a wrong state, which is corrected by emitting an extraneous signal. And when I halt there to debug the situation, it also has to deal with extra signals, which corrects the state, so I cannot inspect the incorrect one. I didn't identify the reason for this behavior yet, so I am not sure whether my exception handling has bugs somewhere or whether Squeak's exception handling is bugged now.
> > > > > >
> > > > > > Kind regards,
> > > > > > Jakob
> > > > > >
> > > > > > Am So., 11. Apr. 2021 um 19:33 Uhr schrieb :
> > > > > >>
> > > > > >> Nicolas Cellier uploaded a new version of Kernel to project The Trunk:
> > > > > >> http://source.squeak.org/trunk/Kernel-nice.1384.mcz
> > > > > >>
> > > > > >> ==================== Summary ====================
> > > > > >>
> > > > > >> Name: Kernel-nice.1384
> > > > > >> Author: nice
> > > > > >> Time: 11 April 2021, 7:33:23.487481 pm
> > > > > >> UUID: ecb5db19-59bc-45f0-85d3-d9296a936a68
> > > > > >> Ancestors: Kernel-mt.1383
> > > > > >>
> > > > > >> Another attempt at fixing #testHandlerFromAction. Unlike previous attempts, this one preserves the expectations of #testHandlerReentrancy.
> > > > > >>
> > > > > >> The solution is to de-activate the handlers as we backtrack the stack, but to reactivate them before performing final exception handling actions (like resuming, resignalling or performing defaultAction). Indeed, those handlers must be able to handle a secondary exception raised in the course of this action.
> > > > > >>
> > > > > >> =============== Diff against Kernel-mt.1383 ===============
> > > > > >>
> > > > > >> Item was changed:
> > > > > >> ----- Method: Context>>handleSignal: (in category 'private-exceptions') -----
> > > > > >> handleSignal: exception
> > > > > >> "Sent to handler (on:do:) contexts only. If my exception class (first arg) handles exception
> > > > > >> and the handler is active then execute my handle block (second arg), otherwise forward
> > > > > >> this message to the next handler context. If none left, execute exception's defaultAction
> > > > > >> (see nil>>handleSignal:)."
> > > > > >>
> > > > > >> | handlerActive val |
> > > > > >> "If the context has been returned from the handlerActive temp var may not be accessible."
> > > > > >> handlerActive := stackp >= 3 and: [(self tempAt: 3) == true].
> > > > > >> (((self tempAt: 1) handles: exception) and: [handlerActive]) ifFalse:
> > > > > >> + [stackp >= 3 ifTrue: [self tempAt: 3 put: false].
> > > > > >> + ^self nextHandlerContext handleSignal: exception].
> > > > > >> - [^self nextHandlerContext handleSignal: exception].
> > > > > >>
> > > > > >> exception privHandlerContext: self contextTag.
> > > > > >> self tempAt: 3 put: false. "disable self while executing handle block"
> > > > > >> val := [(self tempAt: 2) cull: exception]
> > > > > >> + ifCurtailed: [self tempAt: 3 put: true].
> > > > > >> - ensure: [self tempAt: 3 put: true].
> > > > > >> self return: val "return from self if not otherwise directed in handle block"
> > > > > >> !
> > > > > >>
> > > > > >> Item was added:
> > > > > >> + ----- Method: Context>>reactivateHandlers (in category 'private-exceptions') -----
> > > > > >> + reactivateHandlers
> > > > > >> + "Private - sent to exception handler context only (on:do:).
> > > > > >> + Reactivate all the handlers into the chain"
> > > > > >> +
> > > > > >> + self tempAt: 3 put: true. "this is temporary handlerActive in #on:do:"
> > > > > >> + self nextHandlerContext reactivateHandlers!
> > > > > >>
> > > > > >> Item was added:
> > > > > >> + ----- Method: Exception>>reactivateHandlers (in category 'priv handling') -----
> > > > > >> + reactivateHandlers
> > > > > >> + "reactivate all the exception handlers in the context chain"
> > > > > >> + self canSearchForSignalerContext
> > > > > >> + ifTrue: [signalContext nextHandlerContext reactivateHandlers]!
> > > > > >>
> > > > > >> Item was changed:
> > > > > >> ----- Method: Exception>>resignalAs: (in category 'handling') -----
> > > > > >> resignalAs: replacementException
> > > > > >> "Signal an alternative exception in place of the receiver."
> > > > > >>
> > > > > >> + self reactivateHandlers.
> > > > > >> self resumeUnchecked: replacementException signal!
> > > > > >>
> > > > > >> Item was changed:
> > > > > >> ----- Method: UndefinedObject>>handleSignal: (in category 'bottom context') -----
> > > > > >> handleSignal: exception
> > > > > >> + "When no more handler (on:do:) context left in sender chain this gets called. Return from signal with default action.
> > > > > >> + Before doing that, reactivate the handlers so that they can catch eventual secondary exceptions raised by defaultAction."
> > > > > >> - "When no more handler (on:do:) context left in sender chain this gets called. Return from signal with default action."
> > > > > >>
> > > > > >> + ^ exception reactivateHandlers; resumeUnchecked: exception defaultAction!
> > > > > >> - ^ exception resumeUnchecked: exception defaultAction!
> > > > > >>
> > > > > >> Item was added:
> > > > > >> + ----- Method: UndefinedObject>>reactivateHandlers (in category 'bottom context') -----
> > > > > >> + reactivateHandlers
> > > > > >> + "nothing to do for bottom context"
> > > > > >> +
> > > > > >> + ^ self!
> > > > > >>
> > > > > >>
> > > >
> > >
> >
>