Hi Jakob,

Le mar. 20 avr. 2021 à 19:00, Jakob Reschke <jakres+squeak at gmail.com> a écrit :
>
> Hi,
>
> Am Di., 20. Apr. 2021 um 09:16 Uhr schrieb Nicolas Cellier
> <nicolas.cellier.aka.nice at gmail.com>:
> >
> > 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
(<primitive:199>),
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 <jakres+squeak at gmail.com> 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
> > > <jakres+squeak at gmail.com>:
> > > >
> > > > 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
> > > > <jakres+squeak at gmail.com>:
> > > > >
> > > > > 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 <commits at source.squeak.org>:
> > > > >>
> > > > >> 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!
> > > > >>
> > > > >>
> > >
> >
>