Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Builtin thread-local object storage for AsyncTask #1

Merged
merged 5 commits into from
Nov 6, 2016
Merged

Builtin thread-local object storage for AsyncTask #1

merged 5 commits into from
Nov 6, 2016

Conversation

SOF3
Copy link
Member

@SOF3 SOF3 commented Oct 1, 2016

@dktapps deleted my original pull request. Blame him and ask him for description.

@dktapps
Copy link
Member

dktapps commented Oct 1, 2016

#blamedktapps
Sorry this has been such a pain in the butt.

Update AsyncTask.php

Fix syntax error
@SOF3
Copy link
Member Author

SOF3 commented Oct 2, 2016

Rebased commits

@SOF3 SOF3 added the Category: API Related to the plugin API label Oct 4, 2016
@SOF3
Copy link
Member Author

SOF3 commented Oct 4, 2016

@Aericio Please do not post comments irrelevant to this pull request here. If you continue to post improperly, you may be blocked from @pmmp.

Copy link
Contributor

@robske110 robske110 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome :)

@@ -27,7 +27,7 @@
/**
* Class used to run async tasks in other threads.
*
* WARNING: Do not call PocketMine-MP API methods, or save objects from/on other Threads!!
* WARNING: Do not call PocketMine-MP API methods, or save objects (and arrays cotaining objects) from/on other Threads!!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

love this :P

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lol hilarious

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@remotevase please stop spamming on our repo.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, but expect to see more pull requests from me that actually contribute CODE not just templates for issues and pull requests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@remotevase Please immediately stop using pull requests and issues as chatroom or forums, or respective action will be taken.

@pmmp pmmp locked and limited conversation to collaborators Oct 6, 2016
@pmmp pmmp unlocked this conversation Oct 18, 2016
@robske110
Copy link
Contributor

I could try to test this, if you want
-?

@robske110
Copy link
Contributor

The commit name seems to be not very descriptive

@SOF3
Copy link
Member Author

SOF3 commented Oct 26, 2016

@robske110 that's all that this pull request is about. Other changes are just to assist this addition. You don't want to store a value; you just want to retrieve a value, and you do so by storing it first, along with other things like a storage to put these values at. But the main point is that you can use AsyncTask;:fetchLocal.

At least, I think it is good enough just for a commit name to identify a change. Better than this 😉

@robske110
Copy link
Contributor

well, still looks weird for me, but i guess ok

@SOF3
Copy link
Member Author

SOF3 commented Nov 6, 2016

Tested with this script plugin:

<?php

/**
 * @name AsyncTest
 * @author SOFe
 * @main AsyncTest\AsyncTest
 * @api 2.1.0
 * @version 1.0.0
 */

namespace AsyncTest;

class AsyncTest extends \pocketmine\plugin\PluginBase {
        public function onEnable(){
                $asyncTask = new class($this) extends \pocketmine\scheduler\AsyncTask{
                        public function onRun(){
                                sleep(5);
                        }

                        public function onCompletion(\pocketmine\Server $server){
                                $server->getLogger()->info('$asyncTask');
                                var_dump(get_class($this->fetchLocal($server)));
                        }
                };
                $this->getServer()->getScheduler()->scheduleAsyncTask($asyncTask);

                $asyncTask2 = new class($this) extends \pocketmine\scheduler\AsyncTask{
                        public function onRun(){
                                sleep(10);
                        }

                        public function onCompletion(\pocketmine\Server $server){
                                $server->getLogger()->info('$asyncTask2');
                                // var_dump(get_class($this->fetchLocal($server)));
                        }
                };
                $this->getServer()->getScheduler()->scheduleAsyncTask($asyncTask2);
        }
}

Note that anonymous classes are used in this script, and the values to store for an AsyncTask is the $this in the new class($this) part.

Output with ./start.sh --debug.level=2:

[12:07:07] [Server thread/INFO]: Enabling AsyncTest v1.0.0
# Irrelevant lines skipped
[12:07:07] [Server thread/INFO]: Done (1.414s)! For help, type "help" or "?"
[12:07:12] [Server thread/INFO]: $asyncTask
~\plugins\async.php:22:
string(19) "AsyncTest\AsyncTest"
[12:07:22] [Server thread/INFO]: $asyncTask2
[12:07:22] [Server thread/DEBUG]: AsyncTask class@anonymous~\plugins\async.php0000006F77B8036A stored local complex data but did not remove them after completion

*/
public function removeLocalComplex(AsyncTask $for) : bool{
if(isset($this->objectStore[$for])){
Server::getInstance()->getLogger()->debug("AsyncTask " . get_class($for) . " stored local complex data but did not remove them after completion");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Should this be at the WARN level instead?

@SOF3
Copy link
Member Author

SOF3 commented Nov 6, 2016

The pull request is ready for merge. Apart from the question about WARN vs DEBUG above, it is OK for merging. Please review this pull request if you find no problems in the API design.

@robske110
Copy link
Contributor

I think NOTICE would be the right level.

Changed log level of potential memory leak from DEBUG to NOTICE.

This is really wrong. It should be shown even on production servers.
Copy link
Contributor

@robske110 robske110 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now, I'd say ready for merge 👍

* Constructs a new instance of AsyncTask. Subclasses don't need to call this constructor unless an argument is to be passed. ONLY construct this class from the main thread.
* <br>
* If an argument is passed into this constructor, it will be stored in a thread-local storage (in ServerScheduler), which MUST be retrieved through {@link #fetchLocal} when {@link #onCompletion} is called.
* Otherwise, a DEBUG level message will be raised and the reference will be removed after onCompletion exits.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't you change this?

Copy link
Member

@dktapps dktapps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed in detail.

@SOF3 SOF3 merged commit 074583d into pmmp:master Nov 6, 2016
@SOF3 SOF3 deleted the al branch November 6, 2016 14:02
@the-eric-kwok the-eric-kwok mentioned this pull request Dec 15, 2016
@ghost ghost mentioned this pull request Jun 17, 2017
@ZloyNick ZloyNick mentioned this pull request Jul 14, 2017
@pranavbhuv pranavbhuv mentioned this pull request Sep 4, 2017
@FunnyBuddys FunnyBuddys mentioned this pull request Sep 27, 2017
@SuperKali SuperKali mentioned this pull request Nov 25, 2017
@ghost ghost mentioned this pull request Dec 2, 2017
@TwistedAsylumMC TwistedAsylumMC mentioned this pull request Dec 7, 2017
@dries-c dries-c mentioned this pull request Jan 1, 2018
4 tasks
@ghost ghost mentioned this pull request Jan 2, 2018
@nobody5050 nobody5050 mentioned this pull request Jan 6, 2018
@pmmp pmmp locked and limited conversation to collaborators Jan 19, 2018
@dktapps dktapps added Type: Enhancement Contributes features or other improvements to PocketMine-MP and removed Type: Contribution labels Nov 26, 2022
Zwuiix-cmd referenced this pull request in dries-c/PocketMine-MP Aug 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Category: API Related to the plugin API Type: Enhancement Contributes features or other improvements to PocketMine-MP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants