[squeak-dev] The Trunk: Kernel-nice.1384.mcz

Jakob Reschke jakres+squeak at gmail.com
Tue Apr 20 16:59:54 UTC 2021


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.

>
> 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



>
> 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!
> > > >>
> > > >>
> >
>


More information about the Squeak-dev mailing list