ProcessVariable>>valueIfAbsent: bug (and fix)

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

ProcessVariable>>valueIfAbsent: bug (and fix)

Piotr Klibert-2
In the previous thread I mentioned a problem in ProcessVariable I encountered while trying out VisualGST. Turns out I was mistaken, the problem was triggered by something else, as I wasn't able to reproduce it after a fresh rebuild. Still, the bug is there for sure: the `valueIfAbsent` never evaluates the block it gets as an argument. Here's the patch:


    diff --git a/kernel/ProcEnv.st b/kernel/ProcEnv.st
    index d3780748..fa695641 100644
    --- a/kernel/ProcEnv.st
    +++ b/kernel/ProcEnv.st
    @@ -77,7 +77,7 @@ setting for the current process.'>
         valueIfAbsent: aBlock [
            "Return the value of this variable in the current process."
            <category: 'accessing'>
    -       ^Processor activeProcess environment at: self key ifAbsent: [ nil ]
    +       ^Processor activeProcess environment at: self key ifAbsent: aBlock.
         ]

         value [


(the method comment should probably also be changed a bit, now that I look at it...)


Best regard,
Piotr Klibert

Reply | Threaded
Open this post in threaded view
|

Re: ProcessVariable>>valueIfAbsent: bug (and fix)

Holger Freyther
Lovely find! Can you think of a testcase?

thank you!
        holger

> On 8. Jun 2021, at 23:06, Piotr Klibert <[hidden email]> wrote:
>
> In the previous thread I mentioned a problem in ProcessVariable I encountered while trying out VisualGST. Turns out I was mistaken, the problem was triggered by something else, as I wasn't able to reproduce it after a fresh rebuild. Still, the bug is there for sure: the `valueIfAbsent` never evaluates the block it gets as an argument. Here's the patch:
>
>
>    diff --git a/kernel/ProcEnv.st b/kernel/ProcEnv.st
>    index d3780748..fa695641 100644
>    --- a/kernel/ProcEnv.st
>    +++ b/kernel/ProcEnv.st
>    @@ -77,7 +77,7 @@ setting for the current process.'>
>         valueIfAbsent: aBlock [
>            "Return the value of this variable in the current process."
>            <category: 'accessing'>
>    -       ^Processor activeProcess environment at: self key ifAbsent: [ nil ]
>    +       ^Processor activeProcess environment at: self key ifAbsent: aBlock.
>         ]
>
>         value [
>
>
> (the method comment should probably also be changed a bit, now that I look at it...)
>
>
> Best regard,
> Piotr Klibert
>


Reply | Threaded
Open this post in threaded view
|

Re: ProcessVariable>>valueIfAbsent: bug (and fix)

Piotr Klibert-2
Wouldn't something like this suffice?

    TestCase subclass: ProcessVariableTestCase [
        | procvar |

        setUp [
            procvar := ProcessVariable new.
        ]

        testIfAbsent [
            | val |
            val := procvar value.
            self should: [ val isNil ].
            val := procvar valueIfAbsent: [ true ].
            self should: [ val = true ].
        ]
    ]

It should at least catch possible regressions. It would be good to test the behavior with multiple processes, but I think this is outside the scope of this bug: it's reproducible in a single process, and there should be no difference in behavior with multiple processes, as long as the value is not set in any of them.


Actually, I happened upon this when trying to use DynamicVariable (which uses ProcessVariable under the hood). I'm writing a REPL accessible via a SocketServer, for a few reasons (one of them: the local, default REPL seems to block other running processes when waiting for input, not sure if it's a bug?), and I wanted to make Transcript process-local. Each connection lives in its own process, so to make all the output go to the socket instead of stdout, I did something like this:

    DynamicVariable subclass: LocalTranscript [
        LocalTranscript class >> value [
            ^super valueIfAbsent: [ StdOutTranscript ].
        ]
    ]

    Object subclass: DynamicTranscript [
        doesNotUnderstand: aMessage [
            aMessage sendTo: LocalTranscript value.
        ]
    ]

    Smalltalk at: #StdOutTranscript put: (Smalltalk at: #Transcript).
    Smalltalk at: #Transcript put: DynamicTranscript new.

    Object subclass: CommandHandler [
        primHandleCommand: cmd withOutputTo: aTextCollector [
            lastLine := cmd.
            LocalTranscript use: aTextCollector during: [
                self handleEval printString displayNl.
            ]
        ]
    ]

The bug manifested itself when I tried to print something outside of the LocalTranscript>>#use:during: (some logging from the server itself).
 

On Wed, Jun 9, 2021, at 10:18, Holger Freyther wrote:

> Lovely find! Can you think of a testcase?
>
> thank you!
> holger
>
> > On 8. Jun 2021, at 23:06, Piotr Klibert <[hidden email]> wrote:
> >
> > In the previous thread I mentioned a problem in ProcessVariable I encountered while trying out VisualGST. Turns out I was mistaken, the problem was triggered by something else, as I wasn't able to reproduce it after a fresh rebuild. Still, the bug is there for sure: the `valueIfAbsent` never evaluates the block it gets as an argument. Here's the patch:
> >
> >
> >    diff --git a/kernel/ProcEnv.st b/kernel/ProcEnv.st
> >    index d3780748..fa695641 100644
> >    --- a/kernel/ProcEnv.st
> >    +++ b/kernel/ProcEnv.st
> >    @@ -77,7 +77,7 @@ setting for the current process.'>
> >         valueIfAbsent: aBlock [
> >            "Return the value of this variable in the current process."
> >            <category: 'accessing'>
> >    -       ^Processor activeProcess environment at: self key ifAbsent: [ nil ]
> >    +       ^Processor activeProcess environment at: self key ifAbsent: aBlock.
> >         ]
> >
> >         value [
> >
> >
> > (the method comment should probably also be changed a bit, now that I look at it...)
> >
> >
> > Best regard,
> > Piotr Klibert
> >
>
>