Re: [Help-smalltalk] [PATCH] Process

[Gwenaël Casaccio]

On 24/03/2014 10:50, Holger Hans Peter Freyther wrote:
> On Mon, Mar 24, 2014 at 09:22:19AM +0100, Gwenaël Casaccio wrote:
> > +        Termination isNil ifFalse: [ ^ Termination ].
> > +        ^ [
> > +            Termination isNil ifTrue: [ Termination := MethodContext stack: 4 flags: 
> > 6 method: UndefinedObject>>#__terminate ip: 0 sp: -1 ].
> > +            Termination
> > +          ] valueWithoutPreemption
> > +    ]
>
>         ^Termination ifNil: [
>                 Termination := ..
>         ].
>
>         ?
>
> > -    makeUntrusted: aBoolean [
> > -       "Set whether the receiver is trusted or not."
> > -
> > -       <category: 'basic'>
> > -       | ctx |
> > -       ctx := self context.
> > -       [ctx isNil] whileFalse:
> > -               [ctx makeUntrusted: aBoolean.
> > -               ctx := ctx parentContext]
> > -    ]
> let's remove this separately and rhight now? Care to send a patch?
>
> > -       self suspend
> > +       self suspend.
> unrelated patch. Please use git add -p and aim for minimal patches.
>
> > -           | activePriority |
> > -            activePriority := Processor activePriority.
> > +           | oldPriority |
> > +            oldPriority := priority.
> tabs vs. spaces? Separate patch/fix as well.
>
>
> >             priority := anInteger.
> >             "Atomically move the process to the right list, preempting
> >                the current process if necessary."
> > -            self isReady ifTrue: [self resume].
> > +            (myList == (Processor processesAt: oldPriority)) ifTrue: [self 
> > primResume: false].
> Okay. So this has always been broken. Do you want to create a separate
> patch with the old self resume for it?
>
> > -       [aBlock on: ProcessBeingTerminated
> > +       [aBlock on: SystemExceptions.ProcessBeingTerminated

ProcessBeingTerminated is in SystemExceptions namespace otherwhise it's nil

> Why is that?
>
> > -    startExecution: aDirectedMessage [
> Is this code dead? Can we remove it right now?
It's dead
> >       onBlock: aBlockClosure at: aPriority suspend: aBoolean [
> >         <category: 'private'>
> > -       "It is important to retrieve this before we start the
> > -        process, because we want to choose whether to continue
> > -        running the new process based on the *old* activePriority,
> > -        not the one of the new process which is the maximum one."
> > -
> > -       | closure activePriority |
> > -       activePriority := Processor activePriority.
> > -       closure :=
> > -           [[[
> > -               "#priority: is inlined for two reasons.  First, to be able to
> > -                suspend the process, and second because we need to invert
> > -                the test on activePriority!  This because here we may want to
> > -                yield to the creator, while in #priority: we may want to yield
> > -                to the process whose priority was changed."
> > -               priority := aPriority.
> > -               aBoolean
> > -                   ifTrue: [self suspend]
> > -                   ifFalse: [
> > -                       aPriority < activePriority ifTrue: [ Processor yield ] 
> > ].
> > -               aBlockClosure value]
> > -                       on: SystemExceptions.ProcessBeingTerminated
> > -                       do:
> > -                           [:sig |
> > -                           "If we terminate in the handler, the 'ensure' 
> > blocks are not
> > -                            evaluated.  Instead, if the handler returns, the 
> > unwinding
> > -                            is done properly."
> > -
> > -                           sig return]]
> > -                       ensure: [self primTerminate]].
> > -
> > -       "Start the Process immediately so that we get into the
> > -        #on:do: handler.  Otherwise, we will not be able to
> > -        terminate the process with #terminate.  The #resume will
> > -         preempt the forking process."
> > -       suspendedContext := closure asContext: nil.
> > -       priority := Processor unpreemptedPriority.
> > -       self
> > -           addToBeFinalized;
> > -           resume
> > +
> > +       | closure |
> > +       closure := [ [ aBlockClosure value ] ensure: [ self primTerminate ] ].
> > +       suspendedContext := closure asContext: (self class termination) copy.
> > +       priority := aPriority.
> > +       self addToBeFinalized.
> > +        aBoolean ifTrue: [ ^ self ].
> > +       self primResume: false
> tabs vs. spaces. And are you sure we can really simplify it that much? I need to
> have a closer look ath this essential part.

Yes also for another reason with my change the process is always on the 
right priority queue,
the current implementation would only put it when it's yielding or 
scheduled. I strongly
believe that the invariant : ((processor processesAt: priority) select: 
self) should be correct.

> > +TestCase subclass: TestProcess [
> > +        self assert: p_1 isSuspended.
> > +        1 to: 9 do: [ :i | self assert: ((Processor processesAt: i) includes: 
> > p_1) not ].
> Can we have Process return the interval of valid priorities.
>
> > +
> > +        p_1 := [
> > +               ] fork.
> > +
> > +        self assert: p_1 isTerminated.
> hmm. I don't think test will be quite flaky in terms of scheduling and other
> parts. Do you think we can mock/test this in a more reliable way?
I agree it only works when you've a clean image and this is what I want 
otherwise
tests needs to rely on multiple process/scheduling parts that I want to 
tests separately.
> > +
> > +        ok := #test_creation.
> > +        p_1 := [
> > +                 ok := #p_13.
> > +               ] forkAt: 2.
> > +
> > +        self assert: ok = #test_creation.
> same here and probably the other tests.