Re: [Help-smalltalk] [PATCH] Process

[Holger Hans Peter Freyther]

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

Why is that?

> -    startExecution: aDirectedMessage [

Is this code dead? Can we remove it right now?

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

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


> +
> +        ok := #test_creation.
> +        p_1 := [
> +                 ok := #p_13.
> +               ] forkAt: 2.
> +
> +        self assert: ok = #test_creation.

same here and probably the other tests.