Added explicit AsyncTask->storeLocal(), removed AsyncTask->__construct() object storage#1322
Conversation
dktapps
added
BC break
| * WARNING: As of pthreads v3.1.6, arrays are converted to Volatile objects when assigned as members of Threaded objects. | ||
| * Keep this in mind when using arrays stored as members of your AsyncTask. | ||
| * | ||
| * WARNING: Do not call PocketMine-MP API methods from other Threads!! |
There was a problem hiding this comment.
Why? Does "other Threads" include the main thread? Because I call NBT::readCompressed() and Item::nbtDeserialize() in one of my plugins asynchronously.
There was a problem hiding this comment.
A lot of things are not initialized on workers that are initialized on the main thread, which may cause unexpected behaviour. For example, because the ItemFactory is not initialized, calling nbtDeserialize() on a worker will give you a generic Item object no matter what item was stored.
There was a problem hiding this comment.
Oh noes... I messed up in many places.
Far too often I see people using IDEs which generate the constructors for them and then accidentally unintentionally store things in the object store. This parent constructor behaviour is unexpected. If a developer wants to store something, they should now do so explicitly by calling storeLocal().
dktapps
force-pushed
the
explicit-store-local
branch
2 times, most recently
from
426f75c to
ec76bd8
Compare
| * Call this method from {@link AsyncTask#onCompletion} to fetch the data stored in the constructor, if any, and | ||
| * clears it from the storage. | ||
| * Saves mixed data in ServerScheduler's object storage on the main thread. You may use this to retain references to | ||
| * objects or arrays which you need to access in {@link AsyncTask#onCompletion} which cannot be stored as a |
There was a problem hiding this comment.
onCompletion() or onProgressUpdate().
There was a problem hiding this comment.
I think it's best not to introduce more confusion over fetch vs peek. This documents behaviour and the most common use, it's up to developers to decide how to use it.
dktapps
removed
Status: Insufficiently Tested
Status: Incomplete
dktapps
added
Type: Enhancement
3 participants
Introduction
Far too often I see people using IDEs which generate the constructors for them and then accidentally unintentionally store things in the object store. This parent constructor behaviour is unexpected.
If a developer wants to store something, they should now do so explicitly by calling storeLocal(). The intention of this is more clear and there is no doubt about what such a call will do.
This pull request also clarifies the documentation on using the scheduler object storage.
Changes
API changes
storeLocal()instead.Backwards compatibility
This is a backwards-compatibility break for any plugins which used the constructor to store objects.
Tests
TesterPlugin submodule has been updated to reflect the changes in this pull request.