3

I have small code that demonstrate how to perform a race condition in multithreads PHP.

The idea is I and my friend is sharing the pot for cooking. if the pot already have ingredient, so the pot can not cook.

class Pot:

class Pot
{
    public $id;
    function __construct()
    {
        $this->id = rand();
    }

    public $ingredient;

    public function cook($ingredient, $who, $time){
        if ($this->ingredient==null){
            $this->ingredient = $ingredient;
            print "pot".$this->id.'/'.$who." cooking ".$this->ingredient. " time spent: ".$time." \n";
            sleep($time);
            print "pot".$this->id.'/'.$who." had flush ingredient \n";
            $this->ingredient = null;
            
        }else{
            throw new Exception("Pot still cook ".$this->ingredient);
        }
    }
}

class Friend:

class Friend extends Thread
{
    /**
     * @var Pot
     */
    protected $pot;

    function run() {
        Cocking::cleanVegetable("Friend");
        print "Friend will cook: \n";
        $this->pot->cook("vegetable", 'Friend',4);
        Cocking::digVegetable("Friend");
    }

    public function __construct($pot)
    {
        $this->pot = $pot;
    }
}

class My:

class My
{
    /**
     * @var Pot
     */
    private $pot;
    public function doMyJob(){
        Cocking::cleanRice("I");
        print "I will cook: \n";
        $this->pot->cook("rice", "I",10);


        Cocking::digRice("I");
    }

    public function playGame(Friend $friend){
        print "play with friend \n";
    }

    public function __construct($pot)
    {
        $this->pot = $pot;
    }
}

class Coocking:

<?php


class Cocking
{
    static function cleanRice($who){
        print $who." is cleaning rice \n";
    }
    static function cleanVegetable($who){
        print $who."is cleaning vegetable \n";
    }


    static function digRice($who){
        print $who." is digging rice \n";
    }

    static function digVegetable($who){
        print $who." is digging vegetable \n";
    }
}

running script:

require_once "Friend.php";
require_once "My.php";
require_once "Cocking.php";
require_once "Pot.php";

$pot = new Pot();
$friend = new Friend($pot);
$my = new My($pot);

$friend->start();
$my->doMyJob();
$friend->join();
$my->playGame($friend);

that is so wreid that the output never throw exception? that i assume always happen.

root@e03ed8b56f21:/app/RealLive# php index.php
Friendis cleaning vegetable
I is cleaning rice
Friend will cook:
I will cook:
pot926057642/I cooking rice time spent: 10
pot926057642/Friend cooking vegetable time spent: 4
pot926057642/Friend had flush ingredient
Friend is digging vegetable
pot926057642/I had flush ingredient
I is digging rice
play with friend

the Pot had used by me, but my friend still can use it to cook vegetable. that so freak? i expect the result would be:

Friend will cook:
I will cook:
pot926057642/I cooking rice time spent: 10
PHP Fatal error:  Uncaught Exception: Pot still cook rice in /app/RealLive/Pot.php:23
Stack trace:
#0 /app/RealLive/My.php(14): Pot->cook('rice', 'I', 10)
#1 /app/RealLive/index.php(12): My->doMyJob()
#2 {main}
  thrown in /app/RealLive/Pot.php on line 23

ps: my env is

PHP 7.0.10 (cli) (built: Apr 30 2019 21:14:24) ( ZTS )
Copyright (c) 1997-2016 The PHP Group
Zend Engine v3.0.0, Copyright (c) 1998-2016 Zend Technologies

Many thanks from your comment.

Erics Nguyen
  • 370
  • 4
  • 16
  • 1
    Could you try to cut down on the code in your example more, if possible? Or is it already the minimal amount that makes it bug out? Also, if you wrote the full console output that you WOULD have expected (including where you think the exception should have occurred) that would be helpful. – E. T. Oct 11 '20 at 12:32
  • 1
    yes, i had updated all the code, and my expect. it should throw the exception when My and Friend cooking at same time. – Erics Nguyen Oct 11 '20 at 12:44

3 Answers3

2

Your assumption seems to be that your if condition followed by an immediate member assign always needs to run in one go. However, it is entirely possible that Friend runs this line of code in the thread:

if ($this->ingredient==null){

... and concludes to go ahead, but before it reaches the next line that assigns $this->ingredient, execution switches back to My/main thread, where it also gets to this line:

if ($this->ingredient==null){

And since Friend has passed the if but not proceeded to actually assigned the ingredient yet, My can now also pass inside. Whatever runs next doesn't matter, you now got both threads accessing the pot cooking at the same time.

Additional correction/note: it seems like that the example also doesn't work since $this->ingredient isn't a Volatile. However, that would still make it prone to above race condition and hence still a bad idea.

How to do it properly: You really need to use a mutex or synchronized section for proper synchronization. Also, never ever assume threads can't switch in the middle of anywhere, including any two lines like an if followed by a variable assign that was meant as a pair.

Here is the PHP documentation on the synchronized section: https://www.php.net/manual/en/threaded.synchronized.php

E. T.
  • 847
  • 10
  • 26
  • yes, thank you for your information, but i still unobiviuos about the `if($this->ingredient == null)` statement. i have put a sleep when `cooking` to make sure that. the thread My is cooking in a time, and after that thread Friend is comming. but it doesn't make exception. and i increase the thread my and friend to 1000,and run a couple of time, but no exception could be created – Erics Nguyen Oct 12 '20 at 00:36
  • Thread execution can switch any time, not only when a sleep occurs. Your sleep isn't a magical "threads can only switch here" marker. Of course when a thread is sleeping that is a good way to ensure that other threads run, but that doesn't mean they won't ever run at any other points in between. – E. T. Oct 12 '20 at 06:41
  • 1
    I'm honestly not sure if the two threads are seeing the same `$pot` object (it could be a copy-on-write thing because it's not passed as a reference), during my tests I initialized `$fp` in `Friend` constructor, but in `run( ) -> flock()` I was getting an invalid resource, I had to move `$fp` into run(). Having said that, @E.T. is correct, thread scheduling can take place at any time and access to the variable must be protected, synchronized. – Alex Oct 12 '20 at 18:11
  • yes, sleep make all threads be paused, so at this example, it doesn't make sense. – Erics Nguyen Oct 14 '20 at 04:47
  • 1
    can you guy take a look to my answer – Erics Nguyen Oct 14 '20 at 04:48
1

Reading and writing a variable in a multithreaded application does not guarantee synchronization, you need some synchronization mechanism, the variable should be declared atomic to ensure that only one thread at a time can access it for reading or writing, to guarantee consistency between the two threads, or using mutex to synchronize accesses between shared resources (lock / trylock / unlock).

What is currently happening is that the two threads run parallel, the ingredient variable takes random values ​​depending on the order of execution, and when the longest sleep ends the application exits.

In the following example I used flock which is one of the simplest systems to synchronize accesses between multiple processes, during the tests I had problems because probably the Friend constructor is not executed in the same thread as the run function of the same instance ... there are a lot of factors to take into consideration, Thread in php seems deprecated to me and the implementation a bit convoluted compared to languages ​​like C.

class Friend extends Thread
{
    protected $pot;

    function run() {
        $this->pot->cook("vegetable", 'Friend',2);
    }

    public function __construct($pot)
    {
        $this->pot = $pot;
    }
}


class Pot
{
    public $id;
    public $ingredient;

    function __construct()
    {
        $this->id = rand();
    }

    public function cook($ingredient, $who, $time)
    {
        $fp = fopen('/tmp/.flock.pot', 'r');
        if (flock($fp, LOCK_EX|LOCK_NB)) {
            if ($this->ingredient==null){
                $this->ingredient = $ingredient;
                print "pot".$this->id.'/'.$who." cooking ".$this->ingredient. " time spent: ".$time." \n";
                sleep($time);
                print "pot".$this->id.'/'.$who." had flush ingredient \n";
                $this->ingredient = null;
            }
            flock($fp, LOCK_UN);
        } else {
            // throw new Exception("Pot still cook ".$this->ingredient);
            print "ingredient busy for {$this->id}/$who\n";
        }
        fclose($fp);
    }
}

class My
{
    private $pot;

    public function run(){
        $this->pot->cook("rice", "I",3);
    }

    public function __construct($pot)
    {
        $this->pot = $pot;
    }
}

touch('/tmp/.flock.pot');
$pot = new Pot();
$friend = new Friend($pot);
$my = new My($pot);

$friend->start();
sleep(1); // try comment me
$my->run();
$friend->join();
unlink('/tmp/.flock.pot');
Alex
  • 3,264
  • 1
  • 25
  • 40
0

Each thread of program have its own memory. at this example it is Pot and is saved in main memory. and one of threads have read & changed it, and the changed would not reflected to main memory,

So other threads can not be see that changed. so we should make Pot extends Volatile to make the changed can reflected to main memory.

Or make the block synchronized:

if ($this->ingredient==null)
  $this->ingredient = $ingredient;
Erics Nguyen
  • 370
  • 4
  • 16
  • Just marking it as Volatile is still incorrect, see the remark about volatile that I added to my answer. This won't fix the race condition, even if it may work in 99% of the cases, and is therefore a bad idea. Block synchronization would help, though. – E. T. Oct 14 '20 at 07:14
  • 1
    @E.T. yes you are right. `Volatile` is not enough in this case, because both of threads are manipulated on `Pot` – Erics Nguyen Oct 15 '20 at 15:27