fix #580 #681

[sirsnyder]

Cleanup on synchronized or reading a property is inconsistent, therefore cleanup should also happen on writing a property. Moreover the cleanup behavior on synchronized is misleading.

class myRec extends Threaded {
	public $data1;
	public $data2;
}

class storage extends Threaded {

	private $store = array();

	public function addRecord($key,$data) {
        	$this->store->synchronized(function () use ($key, $data)
        	{
			$this->store[$key] = $data;
		});
	}

	public function delAll() {
        	$this->store->synchronized(function ()
		{
			foreach($this->store as $key => $data)
			{
				unset($this->store[$key]);
			}
        	});
	}

	public function countRecs() {
		return count($this->store);
	}
}

class myThread extends Thread
{

	private static $type;

	public function __construct($type,$storage)
	{
		self::$type = $type;
		$this->storage = $storage;
	}

	public function run()
	{

		if (self::$type === 1) {
			while (true) {

				//if($this->storage->countRecs() == 0)
				{
					@$loop++;
					for($i = 0; $i < 30000; $i++)
					{
						$r = new  myRec();
						$r->data1 = 'data' . $i;
						$this->storage->addRecord($loop . '|' . $i, $r);
					}
				}

				sleep(5);

			}
		}

		if (self::$type === 2) {
			while (true) {
				//if($this->storage->countRecs() >= 30000)
				{
					$this->storage->delAll();
				}

				sleep(1);
				print "RECS:".$this->storage->countRecs()."\n";
			}

		}
	}
}

ini_set('memory_limit', '-1');

$storage = new storage();

$threads[0] = new myThread(1,$storage);
$threads[0]->start();

$threads[1] = new myThread(2,$storage);
$threads[1]->start();
$threads[1]->join();

This is a fix for #681 as well.

[tpunt]

It's not immediately clear to me why this is needed - is it related to the memory leaks you're fixing? And do you have a test case for this?

[sirsnyder]

A test case might be difficult. I ran the script above for a certain time and watched the memory consumption. Any idea to trace this in a test script?

I've added pthreads_store_sync at this point to guarantee consistency with the behavior of reading properties. It is not necessary for the memory leak fix itself, just for the cleanup problems in #580.

[tpunt]

Testing the memory leak isn't really necessary, since it's quite unlikely a regression will be introduced here. I just wasn't sure how syncing the pthreads store linked to the memory leaks (it doesn't, but I see that you're using this PR to fix two different bugs).

[krakjoe]

This has the potential to make unsynchronized writes extremely slow ... I don't like it ... would be good to find another way to do whatever you are trying to do ...

[sirsnyder]

Producer/Consumer: Producer Thread A writes properties and never reads them. Consumer Thread B reads/removes properties. Table std.properties in Thread A fills up, endlessly. I could limit pthreads_store_sync to Volatile objects, that might be a solution you like?

[sirsnyder]

I have again tested the script, without the pthreads_store_sync call. I must admit the memory isn't growing. My fault, I must have done something wrong.

[tpunt]

@sirsnyder I'm also having trouble replicating the issue you're receiving without the syncing of the pthreads store. Do you have a smaller reproducing script that explicitly shows a difference with and without the syncing of stores on property writes?

The memory leak fixes look fine to me though.

[sirsnyder]

@tpunt that must have been an erroneous observation in htop. My fault. I've removed the pthreads_store_sync call.

[tpunt]

Merged in 1dfeea7. Thanks!