While at first glance this doesn't seem like a big issue (don't write broken code that assigns wrong types to Threaded objects? ...), it's an issue for user code that usually doesn't use a static analyser to sanitize their code.

This first reared its head in #33 , where automatic coercion of arrays to Volatile broke assumptions made by opcache about property types, causing broken behaviour with opcache enabled and segfaults with JIT enabled on 8.0.
However, I have no doubt it will manifest in interesting ways elsewhere that we haven't seen yet.

This is a problem, because users who are not yet woke enough to use a static analyser will encouter broken behaviour and unexplained segfaults that are impossible for them to debug.

Therefore, it's unfortunately a necessity to implement runtime property type checks to catch this problem at the root.

If you have some task which allocates memory, Worker can crash with segfault in another task. Work only if you have more than one Worker. In production, I have the task which periodically gets from DB data around 300KB and after some hours Worker crashed in another task. I wrote the example of code which usually crashing with segfault in few seconds.

class AllocateMemoryTask extends \Threaded {

    public function run() {
        var_dump(get_class($this));
        $justAllocateManyData = [];
        for ($i = 0; $i < 100000; $i++) {
            $justAllocateManyData[] = 'test';
        }
        $serializeData = serialize($justAllocateManyData);
        $test = [];
        for ($i = 0; $i < 100; $i++) {
            $test[] = unserialize($serializeData);
        }        
    }

}

class SimpleTask extends \Threaded {

    public function run() {
        var_dump(get_class($this));
    }

}

class MyWorker extends \Worker {

    public function run() {
        ini_set("memory_limit", -1);
    }

    public function start(int $options = PTHREADS_INHERIT_CONSTANTS) {
        parent::start($options);
    }

}

$pool = new \Pool(4, MyWorker::class);

for ($i = 0; $i < 4; $i++) {
    $pool->submit(new AllocateMemoryTask());
}

while (true) {
    for ($i = 0; $i < 4; $i++) {
        $pool->submit(new SimpleTask());
    }
    sleep(1);
}

https://github.com/pmmp/pthreads/blob/fork/src/store.c#L89
This function is not thread safe because the hashtable could be modified concurrently. In 4 out of 5 places where this function is used, a race condition can occur because the store is not locked before calling pthreads_store_coerce().
It's possible this might result in rare segfaults when appending to Threaded, but I've never seen this happen.

Classes cached in OPcache shared memory aren't expected to be modified. Threaded::extend() modifies these classes, which aside from being a generic "bad thing", causes very broken behaviour when called after objects of the target classes have been instantiated.

OPcache makes this problem worse because classes cached by OPcache are directly shared between threads, which means that other threads can unexpectedly encounter objects which are now Threaded and weren't before, except that they haven't been correctly initialized.

I don't think there's any circumstances where runtime extension makes any sense. User classes almost always have to be specifically adapted in order to be thread-safe, and internal classes are never safe to directly share anyway (since their internal structures are opaque and typically allocated by ZMM).

Assigning a non-serializable object as a field of Threaded (such as Socket in PHP 8.0) will silently turn into NULL, leading to surprising behaviour.

Right now, Threaded is your only option if you need a thread-safe queue. This is heavily suboptimal for large numbers of items.

Consider the following script:

<?php

declare(strict_types=1);

$buf = new \Threaded();
$t = new class($buf) extends \Thread{
	public \Threaded $queue;
	public bool $shutdown = true;

	public function __construct(\Threaded $t){
		$this->queue = $t;
	}

	public function run(){
		$this->synchronized(function() : void{
			$this->shutdown = false;
			$this->notify();
		});
		$time = 0;
		$read = 0;
		var_dump("child thread running");
		while(!$this->shutdown){
			$this->synchronized(function() : void{
				while($this->queue->count() === 0){
					$this->wait();
				}
			});
			sleep(2);
			while($this->queue->count() > 0){
				$start = hrtime(true);
				$this->queue->shift();
				$time += (hrtime(true) - $start);
				$read++;
				if(($read % 10000) === 9999){
					var_dump("time per element: " . ($time / $read) . "ns");
				}
			}
		}

		while($this->queue->count() > 0){
			$start = hrtime(true);
			$this->queue->shift();
			$time += (hrtime(true) - $start);
		}
	}
};
$t->start();
$t->synchronized(function() use ($t) : void{
	while($t->shutdown){
		$t->wait();
	}
});
var_dump("starting...");

for($i = 0; $i < 1024 * 512; $i++){
	$t->synchronized(function() use ($t, $buf) : void{
		$buf[] = "a";
		$t->notify();
	});
	if(($i % 10000) === 9999){
		var_dump($buf->count());
	}
}

$t->shutdown = true;
$t->join();

You can observe that the performance of shift() rapidly degrades, slowing the reader to a crawl.
If the sleep() call is removed, it's snappy and happy.

This happens because the size of the underlying PHP HashTable balloons to a large size when the consumer is not shifting elements off the queue. When elements are removed, the underlying HashTable doesn't decrease in size, leaving a large number of empty gaps in the queue. This results in this loop taking an extraordinary amount of CPU time to find the first element of the HashTable, causing a huge performance loss to the reader.

This may manifest in PocketMine-MP under lag spike conditions. Because RakLib uses two Threaded objects to send and receive packets from the main thread, a lag spike on either side will cause the size of the Threaded hashtables to blow up and cause significant performance losses. Worse, because the HT is never downsized, the performance loss will persist even after the lag spike is gone.

In addition, this can cause significant performance losses for the writer, because the writer has to acquire a lock on the Threaded in order to add elements to it. The reader executes this extremely costly loop in a lock, which means that the writer will block for a significant amount of time if it happens to write at the same time as the reader is reading.

Since Threaded shared objects became refcounted, worker stack has moved to pthreads_object_t, which is refcounted globally. The thread on which its refcount reaches zero will free the stack.

This is a problem because worker stacks are allocated on the Zend MM, which means they have to be freed by the thread which allocated them. In addition, the worker stacks contain pthreads_zend_object_t references from the origin thread, which the thread will try to free and wind up with a zend_mm_heap corrupted because it doesn't own the object.

This means that if a Worker object is passed to another thread (there isn't really a good reason to do this, so it usually happens by accident, e.g. during statics copying), this crash may occur if the thread tries to free the worker (this can only occur after the main thread already joined and discarded it).

Similar to #28 , this is a solution for a rare-case problem that causes a performance loss for everyone for basically no real benefit.

pthreads_execute_ex()'s purpose is to fix some problems with set_exception_handler() as seen in krakjoe/pthreads#493. For this bug fix, we pay a global performance penalty of 1-2%.

A better solution to this problem would be to resolve the exception propagation issue in pthreads_routine_run_function(). However, this will likely require some complicated shit to manually create Zend VM stack frames to bypass the problem, because zend_call_function() doesn't permit intercepting thrown exceptions.

IS_CONSTANT_AST appears when referencing constants from other constants, from property defaults and also from statics.

This problem manifests in constants and property defaults silently turning into NULL when copied, similarly to krakjoe/pthreads#564.

Non-preloaded opcached classes will be marked with ZEND_ACC_IMMUTABLE, but they won't automatically appear in EG(class_table) for new threads. Currently pthreads assumes that an IMMUTABLE class can always be found by zend_lookup_class(), but this appears not to be the case.

https://github.com/pmmp/pthreads/blob/fork/src/prepare.c#L518

<?php

require __DIR__ . '/vendor/autoload.php';

$t = new class extends \Thread{
        public function run() : void{
                require __DIR__ . '/vendor/autoload.php';
        }
};
$t->start() && $t->join();

This code will crash in any project using Composer + opcache with an error something like this:

Fatal error: Uncaught Error: Class 'ComposerAutoloaderInitc73aff11b28188e2c54bed501f195fbd' not found in /home/dktapps/pm-dev/vendor/autoload.php:7
Stack trace:
#0 /home/dktapps/pm-dev/test.php(7): require()
#1 [internal function]: class@anonymous->run()
#2 {main}
  thrown in /home/dktapps/pm-dev/vendor/autoload.php on line 7

PMMP has no current plans to move to parallel, but now that we're fully responsible for maintaining pthreads, a few changes need to take place to reduce the work necessary to maintain it in the future.

Some proposed changes that come to mind (not exhaustive):

• Drop environment inheritance: no class/function/constant/INI copying. Instead, require a bootstrap file (a la parallel) for autoloading. This would significantly reduce the maintenance burden, at the expense of making it impossible to support anonymous classes for Worker tasks and the like.
• Split up Threaded. Its API is a mess: it currently acts as a base threadsafe class, TS map and TS queue all in one.
• Get rid of the clusterfuck around Threaded immutability: it's confusing and really doesn't make a lot of sense, regardless of the supposed performance benefits.
• Get rid of Collectable: it has exactly zero purpose.

Classes and functions are inseparably linked.

• Methods in classes may declare anonymous functions in their methods which have to be inherited (fixed by SirSnyder a few years ago)
• Functions may declare anonymous classes (currently broken when only PTHREADS_INHERIT_FUNCTIONS is used)
• Functions may refer to classes in their parameter and return typehints (I don't think these currently directly refer to CEs, but if they did we aren't copying them, which would lead to crashes)
• Functions may instantiate classes
• Class methods may call functions
• Methods/functions may also declare named classes and functions (a rare case, but one that currently results in faults in copied code).

The bottom line is that functions and classes are coupled in some (a lot of) cases, and shouldn't therefore be copyable separately.

In particular, the ability to do this has resulted in various faults, assertion failures in php-src, differences in pthreads behaviour with/without opcache, and more besides.

There's a weaker argument for disallowing separate constant inheritance (constants don't circularly depend on anything, but they can be referenced via AST from copied classes).

In the past my proposed solution to this was to copy just the classes and functions needed given a particular class (or function) as a starting point. However, with OPcache in the mix (and especially with JIT), it's difficult to determine what classes or functions are referenced by user code, so we end up needing to copy all of them anyway.

I haven't yet assessed how much of an impact PHP 8 will have on pthreads, but below is a (non-exhaustive) list of things which need to be checked on, in addition to the obvious stuff shown up in the failed builds:

Class copying

• Promoted properties
• Attributes (class, property, method, method parameters)

Function copying

• Attributes
• Nullsafe operator (possibly new opcodes?)

Resources

• Most common resources have been converted to objects in PHP 8.0. It's not clear whether the resource type has been entirely removed, but it seems likely that pthreads' own resource support can be ditched.
• pthreads Socket now conflicts with the one provided by ext-sockets, since ext-sockets now uses objects instead of resources. We should probably drop pthreads Socket, since neither we nor anyone else uses it, and instead work on making ext-sockets Socket class safe to share.

OPcache

• JIT - I currently have absolutely no idea what effects, if any, pthreads will have on JIT. In theory it will have no effects because of immutable classes and functions, but this theory hasn't been tested yet.

Accessing offsets or properties of Threaded objects which are not defined generates a silent NULL result, instead of emitting a warning or throwing an error.

Environment

• PHP: PHP 7.4.16 (cli)
• pthreads: Version => 3.2.1dev
• OS: Linux Ubuntu

Summary

Pool::Submit takes around 0.02sec for each thread. Each iteration of my foreach loop takes 0.002seconds until getting to Pool::Submit line, where time increases 10fold for each iteration, going up from 0.002 to 0.02 average .

Reproducing Code

$ProfilesThreadsPool = new \Pool($ProfilesLimit);
$AllProfilesThreads = [];
$SelectedActivePool = $SelectedProfiles = [];

for ($j = 0; $j < count($OpenTasks); $j++) {
/// Lots of Random Code Before Submitting Thread///
///////////////////////////////////////////////////
$SelectedProfiles = array_merge($SelectedProfiles, array_column($ChunkedProfiles, 'id'));
$CurrentProfileThread = new MultiThreadProfiles($ChunkedProfiles, $ActiveProfile);
$AllProfilesThreads[] = $CurrentProfileThread;
$ProfilesThreadsPool->submit($CurrentProfileThread);
{

Expected Output

Expected thread creation time under 0.002sec

Actual Output

Each thread submit takes 0.02sec .

detach() is useful for background throwaway tasks, as well as getting rid of threads that won't die on proc shutdown (e.g. PM's famously infuriating CommandReader issue on Windows).

Currently known obstacles to supporting detach():

• Child threads currently directly reference non-permanent interned strings from their parent (user class strings). This is problematic because detach() would allow a parent thread to exit without joining the child thread, potentially resulting in UAF on the child.
• detach() would permit Thread objects to be directly disposed without joining, which means that UAF could occur if detach() was used directly after starting a thread (the child thread has to copy some information from the parent thread, like the thread's class). This could be solved by making start() wait until the child thread was ready (but this would incur a performance hit). not a concern

The following code should throw a TypeError, because stdClass is not an instanceof Threaded:

<?php

$w = new Worker();
$w->collector(new stdClass);

Instead, it throws this rather interesting error:

Fatal error: Couldn't find implementation for method stdClass::isgarbage in Unknown on line 0

This is because o is used as an argument parsing code instead of O. o accepts any object, as seen in the new ZPP API (which is statically analysable): https://github.com/php/php-src/blob/master/Zend/zend_API.h#L1641

If you know how to write phpt tests, please open a PR with environment information, and your test, this will cause CI to test your issue before a human is able to look

Environment

• PHP: 7.3.0alpha2
• pthreads: c51c509
• OS: linux

Summary

This odd behaviour is seen by running the chunk.phpt test.

After extensive debugging, I came to the conclusion that a behavioural change in zend_hash_move_forward_ex() is responsible for this bug (see php/php-src@d7f2dc4#diff-fd78a0a3f78ea28c6907f907f25b908eR2139). The explanation is that iterating forwards on a hashtable from a non-existing position at 0 causes 1 to be skipped because when index 0 is given and does not exist, it now seeks to index 1, which is then incremented in the loop below to 2, causing 1 to be skipped.

This behaviour could be corrected in a few ways:

• by using _zend_hash_get_first_pos here
• by moving the zend_hash_move_forward_ex call before deleting the offsets (might break other behaviour).

A summary isn't always necessary, code > words ... delete this section, as applicable.

Reproducing Code

$threaded = new Threaded;

for($i = 0; $i < 20; ++$i){ $threaded[] = true; }

var_dump($threaded->chunk(10));

Expected Output

array(10) {
  [0]=>
  bool(true)
  [1]=>
  bool(true)
  [2]=>
  bool(true)
  [3]=>
  bool(true)
  [4]=>
  bool(true)
  [5]=>
  bool(true)
  [6]=>
  bool(true)
  [7]=>
  bool(true)
  [8]=>
  bool(true)
  [9]=>
  bool(true)
}

Actual Output

array(10) {
  [0]=>
  bool(true)
  [2]=>
  bool(true)
  [3]=>
  bool(true)
  [4]=>
  bool(true)
  [5]=>
  bool(true)
  [6]=>
  bool(true)
  [7]=>
  bool(true)
  [8]=>
  bool(true)
  [9]=>
  bool(true)
  [10]=>
  bool(true)
}

Currently pthreads has some lifetime issues mostly surrounding child-to-parent-thread data passing of stuff like classes and functions. This is largely because of request-local interned strings which are used for basically all user code.

Request-local interned strings behave mostly the same as regular interned strings (they are not refcounted). However, instead of living for the lifespan of the PHP process like permanent interned strings do, they only live until the end of the current request. In the context of pthreads, a request == a thread.

This has manifested in problems with Threaded object connections when passing Threaded objects from a child thread to a parent thread which doesn't know about the object's class. It assumes that because IS_STR_INTERNED is set on the GC flags, it's safe to use the strings in the class without copying them, but this isn't true, because they might be destroyed when the creating thread exits if they are request-local.

This has been worked around by hard-copying interned strings in some cases, but it's still not a very good solution, because in the cases where strings are not hard copied, there's an assumption about the origin context's lifetime, which imposes various restrictions (for example, it's not practical to support pthread_detach().)

The solution to this problem is to do something similar to OPcache, and implement a custom thread-safe interned strings buffer. We can assign custom handlers to create request-local interned strings in such a way that they'll persist until the process end, making them safe to reuse unconditionally.

https://github.com/php/php-src/blob/a1fee87c9a8a6ed58f2a8270c0a9e61a674d32db/ext/opcache/ZendAccelerator.c#L2686

Travis CI misses a lot of bugs that show up in local testing because Travis PHP binaries are built in release mode. I locally use ASan and PHP built in debug mode, which causes a lot more problems to show up.

A very large amount of pthreads' code is dedicated solely to copying code from the origin thread.

Right now, class copying takes place on 3 occasions:

1. When a thread starts, all classes, functions and constants loaded on the parent thread (depending on PTHREADS_INHERIT_* flags, default is ALL) are copied to the child thread.
2. When a thread starts, the thread's own class is copied from the parent (if it wasn't already). This always happens, regardless of the INHERIT flags used.
3. When a task is run on a Worker, the task's class is copied from the parent. This always happens, regardless of the INHERIT flags used.
4. When a Threaded object assigned by thread1 is read by thread2, the Threaded object's class is copied from thread1 to thread2. This always happens, regardless of the INHERIT flags used.

Reasons to stop doing this:

• It's extremely costly (adding significant delays to thread start)
• It can't make clean or complete copies. Defaults for stuff like static properties are unavailable after those properties are modified, and may contain data types that can't be easily copied (e.g. objects, arrays, resources). (no longer applicable to PHP 8.1)
• It's a massive maintenance burden, accounting for the vast majority of compatibility issues that arise on any new PHP version.
• It wastes a ton of memory - the copied classes might not be needed by the target thread anyway.
• PocketMine-MP makes little to no usage of this code, preferring to rely on autoloading for better reliability, predictability, performance and memory usage (especially, opcache keeps memory usage low with cached code, if autoloading is used).
• Most projects use Composer for autoloading, in which case it's preferable to have something like #23
• It worsens stability - there's so many things that can and do go wrong due to small undocumented changes in php-src.

Reasons to keep it:

• Anonymous Threaded classes can't be supported without it, since they can't be autoloaded. Dropping code copying means we would no longer be able to have anonymous Threaded classes. I don't consider this to be a big enough reason to keep the gigantic maintenance burden of class copying around.

Obstacles

• Point 2 above is a problem, since there's currently no other way for the Thread class to get into the actual thread right now other than having it copied from the parent. This could be solved by implementing something like #23 .

Compatibility

Since PocketMine-MP doesn't currently use code inheritance at all (PTHREADS_INHERIT_NONE or PTHREADS_INHERIT_INI is typically used), it currently relies almost entirely on the autoloader anyway, so this change would have basically zero impact on compatibility.

We're not currently paying any attention to the IMMUTABLE flag if it's set, which might cause unintended side effects on copied functions which inherit this flag when it's not expected.

This is subject to the usual interned strings rules.

pthreads needs to be aware of preloaded classes (they shouldn't be copied). It may also be possible to leverage preloaded classes to reduce or even eliminate the need for class copying.

The following code causes a deadlock, because shutdown functions run inside pthreads_globals_lock():

<?php

require 'vendor/autoload.php';

class T extends \Thread{

	public bool $shutdown = false;

	public function __construct(private \Threaded $dummy){}

	public function run(){
		register_shutdown_function(function() : void{
			var_dump("trying to read dummy in shutdown function...");
			var_dump($this->dummy); //deadlock
			var_dump("dummy read done!");
		});

		while(!$this->shutdown){
			$this->wait();
		}
	}
}

$dummy = new \Threaded();
$thread = new T($dummy);
$thread->start();

$dummy->synchronized(function() use ($thread) : void{
	$thread->shutdown = true;
	$thread->notify();
	var_dump("trying to join thread ...");
	$thread->join();
	var_dump("done!");
});

Function cache uses function_add_ref() to add references to functions that have been copied, which messes with the runtime cache. In 7.3, it just destroyed the cache reference, but on 7.4 it reinitializes it and assumes that the HEAP_RT_CACHE is not set.

In addition, pthreads_prepared_entry_function_prepare() doesn't respect this flag either, and forcibly sets the flag and allocates rt cache on the heap. I don't know exactly why this breaks stuff, but it's causing some tests to fail under ASAN, even though the correct flags are set - it seems like PHP reallocates it on its own without changing the flags somewhere.

<?php

declare(strict_types=1);

$t = [];
$t[0][0] = 1;
$t[0][1] = 2;

var_dump($t);

produces

array(1) {
  [0]=>
  array(2) {
    [0]=>
    int(1)
    [1]=>
    int(2)
  }
}

vs

<?php

declare(strict_types=1);

$t = new \Threaded();
$t[0][0] = 1;
$t[0][1] = 2;

var_dump($t);

which produces

Notice: Indirect modification of overloaded element of Threaded has no effect in C:\Users\dylan-work\Documents\projects\pocketmine-mp\test2.php on line 25

Notice: Indirect modification of overloaded element of Threaded has no effect in C:\Users\dylan-work\Documents\projects\pocketmine-mp\test2.php on line 26
object(Threaded)#1 (0) {
}

krakjoe/pthreads#724

Despite krakjoe's claim that this is expected behaviour, I think it's severely problematic:

• Memory issues on workers will crash all tasks that get submitted, without any indication to the main thread what's going on. This can happen due to, for example, overpopulation of thread-local storage.
• register_shutdown_function() is unable to capture fatal errors because it's never called (?!?!?!?!?!?)
• The parent thread has no idea that there might be a persistent state issue with a worker, causing it to continue to post tasks that subsequently fail too

One possible option is to just scrap Worker and reimplement it in userland (this isn't actually terribly difficult).

As described by this test, == between two Threaded objects actually tells whether the two objects are the same underlying Threaded object.

This is no longer useful because we don't form connection chains, so a Threaded object will always be referred to by the same Zend object.

In addition, == is actually expected to recursively compare properties. While this is obviously insanely inefficient, it's what's expected by PHP. This is yet another stroke of krakjoe genius repurposing language features for things they weren't designed for.

pthreads_store currently malloc()s a new pthreads_storage for every item, regardless of type. This is wasteful because the underlying HashTable which pthreads uses for shared properties uses Z_PTR to wrap around these storages.

Thus, the access for an integer becomes

(Z_PTR_P(zend_hash_find(propName)))->lval

when instead, it could take one less allocation and also one less indirection by storing a ZVAL_LONG in the HashTable directly, instead of a ZVAL_PTR->pthreads_storage.

This would both improve performance (slightly) and reduce memory usage of Threaded objects.

This is useful for status reporting in a way that isn't system-dependent and doesn't require the user to use unreliable methods of counting threads.

This causes some unusual side effects, such as code incorrectly believing that it's running on the main thread.

Currently, the lifetime of a Threaded object is whatever the lifetime of its initial reference on the parent thread happens to be. This is problematic because Threaded objects might unexpectedly become unusable on another thread, leading to a segmentation fault when accessed. In addition, such behaviour is counter-intuitive.

This issue proposes that a thread-safe reference counter be added to threaded object base which keeps the store alive if non-zero. This reference counter would be incremented once for each active Threaded object which connects to it.

Currently the copying routine for static arrays works something like this:

• Serialize
• Unserialize
• Recursively remove complex refs from unserialized array

This is problematic because serialization of an object which is not in its owning thread context can lead to memory issues (most notably property table lazy-initialization to a memory block owned by a different thread), but also because it's inefficient and also subject to recursion issues.

This issue proposes that a proper method for copying statics should be implemented without serializing, which just skips complex refs to begin with, sidestepping the problem.

Currently, pthreads makes copies of classes when a new thread is started. This is a problem for numerous reasons:

• Static properties may have been modified, and since the default values of static properties are not retained (unless opcache is used), we cannot make clean copies of classes. This forces us to copy objects like Threaded objects and other things which might not be copyable, like resources. Resolved since PHP 8.1
• In PHP 8.1, we will need to copy enum classes, which have object constants. This is problematic for the same reason - we can't conveniently copy objects (although we could copy the AST that instantiates them, but again, without opcache, this is typically not retained). Resolved by other means
• Static variables of functions may have been modified (same problem as with static properties).

We can avoid all of these issues by copying the class to persistent memory when it is loaded, exactly the same way that OPcache does.

https://github.com/pmmp/pthreads/blob/39e8ad18c3978cfc8b00677576ccbafd9d88668d/src/store.c#L374

pthreads_store_sync() is not thread-safe, but is called without any mutex lock here. This could lead to unexpected behaviour or possible faults.

Interned string hard copies are necessary in some cases to avoid referencing strings from a context that might be shorter-lived than our own. However, if a string is flagged with IS_STR_PERMANENT, it will live for the lifetime of the process.

This flag is most commonly used by OPcache to indicate strings which are interned on SHM.

When foreaching a Threaded object, the resulting Threaded objects (if any were present) will be different than the ones given by reading a property or dimension. This is because this code doesn't check the local property table for an already existing Threaded object before trying to create another one.

This manifested as a deadlock in PM code. A separate issue (shutdown functions being run inside pthreads_globals_lock) means that threads can't connect to Threaded objects while another thread is executing a shutdown function.

An interesting bug recently surfaced in PM: pmmp/PocketMine-MP@61077c4

In my opinion, a worker should always be considered "running" as long as it hasn't shut down, because internally what's essentially happening is this:

while(!$this->shutdown && $this->queue->count() > 0){
    $task = $this->queue->synchronized(function() : void{
        while($this->queue->count() === 0){
            $this->queue->wait();
        }
        return $this->queue->shift();
    });
    $task->run();
}

If a Worker was implemented in userland, this wouldn't be a problem.

Recreating the entire API of ext-sockets is undesirable due to the amount of work it requires, both for the initial implementation and for long-term maintenance.

Fortunately, it's possible (and quite easy) to properly support exporting sockets to other threads, since we can just make ThreadedSocket wrap around a Socket resource (or object in PHP 8.0), export the underlying socket (it's just an integer) to pthreads_storage, and then reimport it into a new resource on the child thread.

This would enable leveraging the existing socket_* API without needing to recreate them, lowering the maintenance burden.

PocketMine doesn't currently use this because it's too unreliable (the tests also show that opcache breaks it by optimising away the opcodes, because it thinks they are redundant).
Considering that opcache already breaks this malfeature, and almost all servers are using PHP with opcache, it's safe to assume that the usefulness of this feature is close to zero.

In addition, the automatic coercion from array to Volatile is unexpected in almost all cases, and breaks various things such as json_encode($threaded->linearArray).

Scrapping automatic Volatile coercion itself may be the best way forward. Objects still suffer from being unable to update threaded->field->subfield, which there's nothing at all we can do about; at least for array offset accesses we can emit a warning if trying to write an array offset on a Threaded. Instead, introducing Volatile::coerce(array $array) might be a better idea.

These overloaded opcodes are set to work around a bug that occurs because of another bug that occurs because of shitty internal design, as seen in this test case: https://github.com/krakjoe/pthreads/blob/4d1c2483ceb459ea4284db4eb06646d5715e7154/tests/object-cast.phpt

In a recent benchmark of PM lighting, I noticed that the RECV opcode handler is taking up 5.05% of the total CPU time, which I believe is because there were objects being passed around to parameters. This is a pretty nasty performance loss for a shitfeature workaround that nobody uses anyway.

This could be resolved if the way global object connections work is overhauled, so that we don't try to make connections to connections.

Currently we store all Threaded properties indexed by name. However, for all predefined properties, Zend computes a property offset, which is a number that refers to the described property.

Aside from being faster than hashing strings, this also reduces memory usage.

In addition, use of properties on Threaded objects currently results in a string copy on every write, because we cannot assume that a string provided by the writing thread will continue to exist for long enough for another thread to use it. Use of property offsets would bypass this problem for non-dynamic properties, which are 99% of the use case.

Environment

• PHP: 8.0.5 (cli) (built: May 30 2021 12:02:37) ( ZTS Visual C++ 2019 x64 )
• pthreads: 3.2.1dev
• OS: Windows 64-bit

Introduction

The memory leak problem occurs when JIT is enabled for PocketMine-MP server especially for the PopulationTasks in one of the AsyncWorker. Unfortunately I am unable to reproduce this memory leak outside PopulationTask and this leak is producible without any plugins used in the server. The leak seems to start when the chunk is being generated(?)

This could either be PocketMine-MP issue but I do not think that this is the case since it is working just fine without JIT enabled.

Reproducing Code

It can be reproduced using my debug branch of PocketMine-MP 4.0 software. The only changes is that, there will be memory_get_usage() in certain parts where I believe the memory leaks begin.

opcache.jit will be set to 1205

Expected Output

This is what would happen if JIT is not enabled. No abnormal memory usage, everything is fine.
non-jit.txt

Memory usage in bytes

Memory usage: 23054712 23146400 23146400 23146400  31457280 31457280 31457280 31457280
Memory usage: 23054712 23138208 23138208 23138208  31457280 31457280 31457280 31457280
Memory usage: 23054712 23138208 23138208 23138208  31457280 31457280 31457280 31457280
Memory usage: 23054712 23146400 23146400 23146400  31457280 31457280 31457280 31457280
Memory usage: 23054712 23138208 23138208 23138208  31457280 31457280 31457280 31457280
Memory usage: 23054712 23138208 23138208 23138208  31457280 31457280 31457280 31457280
Memory usage: 23054712 23138208 23138208 23138208  31457280 31457280 31457280 31457280
Memory usage: 23054712 23146400 23146400 23146400  31457280 31457280 31457280 31457280
Memory usage: 23054712 23138208 23138208 23138208  31457280 31457280 31457280 31457280
Memory usage: 23054712 23149224 23149224 23149224  31457280 31457280 31457280 31457280
Memory usage: 23054712 23138208 23138208 23138208  31457280 31457280 31457280 31457280
Memory usage: 23054712 23137784 23137784 23137784  31457280 31457280 31457280 31457280
Memory usage: 23054712 23138208 23138208 23138208  31457280 31457280 31457280 31457280
Memory usage: 23054712 23146400 23146400 23146400  31457280 31457280 31457280 31457280
...

Actual Output

This is when JIT is enabled, the memory usage gradually increased until the worker ran out of memory and died.
jit.txt

Memory usage in bytes

Memory usage: 29945536 30041320 30041320 30041320  39845888 39845888 39845888 39845888
Memory usage: 29957824 30061800 30061800 30061800  39845888 39845888 39845888 39845888
Memory usage: 29966016 30069144 30069144 30069144  39845888 39845888 39845888 39845888
Memory usage: 29974208 30071264 30071264 30071264  39845888 39845888 39845888 39845888
Memory usage: 29986496 30090472 30090472 30090472  39845888 39845888 39845888 39845888
Memory usage: 29998784 30094992 30095840 30095840  39845888 39845888 39845888 39845888
Memory usage: 30011072 30110952 30110952 30110952  39845888 39845888 39845888 39845888
Memory usage: 30019264 30115472 30115896 30115896  39845888 39845888 39845888 39845888
Memory usage: 30031552 30135528 30135528 30135528  39845888 39845888 39845888 39845888
Memory usage: 30039744 30132280 30132704 30132704  39845888 39845888 39845888 39845888
Memory usage: 30047936 30152760 30152760 30152760  39845888 39845888 39845888 39845888
Memory usage: 30060224 30157280 30157280 30157280  39845888 39845888 39845888 39845888
Memory usage: 30072512 30172392 30172392 30172392  39845888 39845888 39845888 39845888
...

When copying a class from a thread other than its creator, faults may occur if the class was autoloaded by the child thread and it had statics. This is because the child thread might have a larger map_ptr region than the parent thread.

To address this, we need to know the thread that data is originating from, in order to complete the copy correctly.

https://github.com/pmmp/pthreads/blob/b81ab29df58fa0fb239a9d5ca1c2380a0d087feb/src/prepare.c#L142
This map_ptr is never initialized for prepared, so it will point to an unknown address.
This is at best going to segfault, and at worst write the address of prepared->default_static_members_table to some random place in memory.
This issue was written at 2am, so it's possible I missed something obvious.

0bbe8ea partially resolved the issue of closures getting destroyed, but not entirely, because:

• in some cases the exact same closure ends up pointing to a different zend_function (???)
• in other cases, it might be dereferenced by another thread whose pointer won't match
• opcache might break things

self::class in a copied closure will refer to the Thread that deserializes it, or null for a closure that was copied from a static property.
static::class refers to the self scope.

As per title. Closure scopes are not amended after copying, so they'll refer to their original scope classes on the creator thread, potentially leading to faults (again, child to parent copying is the most likely cause).