Document some bad practices that should be avoided: Fixes #541 #558
Conversation
|
A preview for this pull request is available at https://cdn.rawgit.com/Spongy/SpongeDocs-PRs/c539d17/. Here are some links to the pages that were modified:
Since the preview frequently changes, please link to this comment, not to the direct url to the preview. |
This isn't "bad practice", it needs to be absolutely front and centre not tucked away in a bad practices section. It should be included as a fundamental point when discussing both the scheduler (it probably already is) and event listeners. |
source/plugin/practices.rst
Outdated
| Bad Practices | ||
| ============= | ||
|
|
||
| These should practices should be avoided. As they lead to memory leaks (``OutOfMemoryError``), lags or inconsistencies. |
There was a problem hiding this comment.
Make this one sentence (...avoided, as ...).
They can lead to memory leaks - unless you're 100% certain they will?
Lag does not need to be a plural.
source/plugin/practices.rst
Outdated
| * ``Collections`` | ||
| * ``Maps`` | ||
|
|
||
| should **NEVER** be stored or be cached in plugins. |
There was a problem hiding this comment.
The second be is redundant.
source/plugin/practices.rst
Outdated
| * The references prevent proper garbage collection | ||
| * The instances might no longer be valid | ||
|
|
||
| This can easily be avoided by using the corresponding snapshots or saving the UUID of given the instances and request a |
There was a problem hiding this comment.
To keep tense consistent, use "requesting" instead of "request".
source/plugin/practices.rst
Outdated
|
|
||
| Executing some IO operations such as loading a config/data file or checking for updates/connecting to a website takes | ||
| much time and greatly affects the TPS on the server. Such tasks should be either done in their own threads or using the | ||
| inbuilt scheduller's async feature. |
There was a problem hiding this comment.
My most favorite mistake...Fixed.
source/plugin/practices.rst
Outdated
| ~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| Executing some IO operations such as loading a config/data file or checking for updates/connecting to a website takes | ||
| much time and greatly affects the TPS on the server. Such tasks should be either done in their own threads or using the |
There was a problem hiding this comment.
Move either ie. "Such tasks should be done either in their own thread or ..."
I'd also like to see a comma after "threads".
|
@Inscrutable Thanks for reviewing all my PRs. |
|
@Mumfrey I made the warning stronger and also added it to the scheduler docs. Is this sufficient or shall I change it some more? IMO its a good advice to have that warning in the best practise section as well or shall I remove it from there to reduce redundancies? (I recommend using async for lengthy tasks a few lines above so it might be useful to also outline the limitations there) |
|
Reading the plugins config on a different thread? |
Inscrutable
added
the
needs review
|
I'm hoping someone (else) can review this dusty PR so we can merge it or bury it. |
|
@MarkL4YG I added a comment concerning the IO during the server startup. Do you consider this enough? @Inscrutable Objections? |
|
@ST-DDT Yes, I think that'll do. Be aware that in the preview the page is still named "Best practices" while the subsection headline "Bad practices" is not really showy compared to the other titles. |
|
Should I move it to its own page? Or is there a way to increase the increase the showiness? |
|
Maybe order it the way "Setting up your project" is ordered and call the page "Practices" or sth like that. |
source/plugin/practices.rst
Outdated
| Bad Practices | ||
| ============= | ||
|
|
||
| These should practices should be avoided, as they can lead to memory leaks (``OutOfMemoryError``), lag or |
source/plugin/practices.rst
Outdated
|
|
||
| Executing some IO operations such as loading a config/data file or checking for updates/connecting to a website takes | ||
| much time and greatly affects the TPS on the server. Such tasks should be done either in their own threads, or using the | ||
| inbuilt scheduler's async feature. It is perfecty fine to load required files/config on the main thread during server |
There was a problem hiding this comment.
"perfectly". It might also be better to phrase this starting with "However, ", and omit the "though" at the end.
source/plugin/practices.rst
Outdated
| For more details refer to the :doc:`scheduler` docs. | ||
|
|
||
| If this is done wrong, you will see clients timing out or the server's watchdog might kill the server. |
There was a problem hiding this comment.
I suggest "If this is done incorrectly, you may see clients time out, or the watchdog may even kill the server."
source/plugin/practices.rst
Outdated
|
|
||
| Accessing game objects outside of the main thread can lead to crashes, inconsistencies and various other problems and | ||
| should be avoided. If you have a lengthy operation on a different thread use the :doc:`scheduler` to execute the apply | ||
| part on the main thread. If you want to use an game object in a different thread use a snapshot of the instance or a |
There was a problem hiding this comment.
"to execute the apply part" -> something like "to make changes to such game objects" (it's not clear what the "apply part" means here)
ST-DDT
force-pushed
the
BestPractises
branch
from
79674b9 to
0b26cc1
Compare
|
@Inscrutable @dualspiral @MarkL4YG Fixed and split into two pages for better overview. |
source/plugin/practices/index.rst
Outdated
| Practices | ||
| ========= | ||
|
|
||
| These section describes some best practices that should be followed and some bad practices that should be avoided. |
|
More changes needed or is it good now? |
Inscrutable
removed
input wanted
Documents the following issues new developers might make.