Document some bad practices that should be avoided: Fixes #541 #558

ST-DDT · 2016-12-19T23:09:17Z

Documents the following issues new developers might make.

Storing References (NEVER Store direct references to entities in Sets/Maps in plugins #541)
IO on the main thread
Accessing game objects outside the main thread

Spongy · 2016-12-19T23:10:40Z

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.

Mumfrey · 2016-12-19T23:39:30Z

Accessing game objects outside the main thread

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.

Inscrutable

Quibbles. In addition, you may need to axe the last section on Game Objects.

Inscrutable · 2016-12-20T08:25:42Z

source/plugin/practices.rst

+Bad Practices
+=============
+
+These should practices should be avoided. As they lead to memory leaks (``OutOfMemoryError``), lags or inconsistencies.


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.

Inscrutable · 2016-12-20T08:26:43Z

source/plugin/practices.rst

+    * ``Collections``
+    * ``Maps``
+
+should **NEVER** be stored or be cached in plugins.


The second be is redundant.

Inscrutable · 2016-12-20T08:28:16Z

source/plugin/practices.rst

+* 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


To keep tense consistent, use "requesting" instead of "request".

Inscrutable · 2016-12-20T08:28:55Z

source/plugin/practices.rst

+
+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.


Typo. It's scheduler

My most favorite mistake...Fixed.

Inscrutable · 2016-12-20T08:30:46Z

source/plugin/practices.rst

+~~~~~~~~~~~~~~~~~~~~~
+
+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


Move either ie. "Such tasks should be done either in their own thread or ..."
I'd also like to see a comma after "threads".

ST-DDT · 2016-12-20T13:54:06Z

@Inscrutable Thanks for reviewing all my PRs.

ST-DDT · 2016-12-20T14:22:08Z

@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)

MarkL4YG · 2017-02-06T12:37:32Z

Reading the plugins config on a different thread?
I do think that's a bit overpowered since reading configs should be done after initialization and reloading during operation itself is already considered bad practice - The plugin should simply disallow it entirely or warn the administrator about that.
I also think there's no point in detaching from the main thread during initialization since the server shouldn't be operating while plugins (may) still initialize.

Inscrutable · 2017-07-29T13:44:21Z

I'm hoping someone (else) can review this dusty PR so we can merge it or bury it.

ST-DDT · 2017-09-23T11:29:12Z

@MarkL4YG I added a comment concerning the IO during the server startup. Do you consider this enough?
If yes, then I'm going to merge this.

@Inscrutable Objections?

MarkL4YG · 2017-09-23T11:42:43Z

@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.

ST-DDT · 2017-09-23T11:47:17Z

Should I move it to its own page? Or is there a way to increase the increase the showiness?

MarkL4YG · 2017-09-23T13:25:22Z

Maybe order it the way "Setting up your project" is ordered and call the page "Practices" or sth like that.

Inscrutable

Mostly just more quibbles.

Inscrutable · 2017-09-26T08:54:11Z

source/plugin/practices.rst

+Bad Practices
+=============
+
+These should practices should be avoided, as they can lead to memory leaks (``OutOfMemoryError``), lag or


too many "should"s.

Inscrutable · 2017-09-26T08:55:34Z

source/plugin/practices.rst

+
+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


"perfectly". It might also be better to phrase this starting with "However, ", and omit the "though" at the end.

Inscrutable · 2017-09-26T08:59:59Z

source/plugin/practices.rst

+
+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.


I suggest "If this is done incorrectly, you may see clients time out, or the watchdog may even kill the server."

Inscrutable · 2017-09-26T09:00:48Z

source/plugin/practices.rst

+
+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


"a game object"

"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 · 2017-09-26T21:49:15Z

@Inscrutable @dualspiral @MarkL4YG Fixed and split into two pages for better overview.

Inscrutable · 2017-09-27T12:34:01Z

source/plugin/practices/index.rst

+Practices
+=========
+
+These section describes some best practices that should be followed and some bad practices that should be avoided.


"This section"

ST-DDT · 2017-10-04T20:11:13Z

More changes needed or is it good now?

ST-DDT added the input wanted We would like to hear your opinion label Dec 19, 2016

ST-DDT self-assigned this Dec 19, 2016

Inscrutable reviewed Dec 20, 2016

View reviewed changes

windy1 force-pushed the master branch from 179fd87 to fafc84a Compare December 30, 2016 16:39

ST-DDT added 5 commits January 7, 2017 12:21

Document some bad practices that should be avoided: #541

7743cac

Minor improvements

c6a915a

Fix typo

06d2662

Fixed mistakes

e70c40d

Stronger "Accessing game objects outside the main thread" warning

8b47c08

ST-DDT force-pushed the BestPractises branch from dff12f6 to 8b47c08 Compare January 7, 2017 11:22

stephan-gh changed the base branch from master to stable February 4, 2017 09:58

Inscrutable added the needs review The submission is ready and needs to be reviewed label Jul 29, 2017

ST-DDT added 2 commits September 23, 2017 13:11

State exception during server startup

8d84dcc

Merge branch 'stable' into BestPractises

ba97be9

Inscrutable reviewed Sep 26, 2017

View reviewed changes

Split practices

0b26cc1

ST-DDT force-pushed the BestPractises branch from 79674b9 to 0b26cc1 Compare September 26, 2017 21:43

Inscrutable reviewed Sep 27, 2017

View reviewed changes

Fix typo

03e7eba

Inscrutable removed input wanted We would like to hear your opinion needs review The submission is ready and needs to be reviewed labels Oct 5, 2017

Inscrutable merged commit 4313b03 into stable Oct 5, 2017

ST-DDT deleted the BestPractises branch November 12, 2017 22:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document some bad practices that should be avoided: Fixes #541 #558

Document some bad practices that should be avoided: Fixes #541 #558

ST-DDT commented Dec 19, 2016

Spongy commented Dec 19, 2016 •

edited

Mumfrey commented Dec 19, 2016

Inscrutable left a comment

Inscrutable Dec 20, 2016

ST-DDT Dec 20, 2016

Inscrutable Dec 20, 2016

ST-DDT Dec 20, 2016

Inscrutable Dec 20, 2016

ST-DDT Dec 20, 2016

Inscrutable Dec 20, 2016

ST-DDT Dec 20, 2016

Inscrutable Dec 20, 2016

ST-DDT Dec 20, 2016

ST-DDT commented Dec 20, 2016

ST-DDT commented Dec 20, 2016 •

edited

MarkL4YG commented Feb 6, 2017

Inscrutable commented Jul 29, 2017

ST-DDT commented Sep 23, 2017

MarkL4YG commented Sep 23, 2017

ST-DDT commented Sep 23, 2017

MarkL4YG commented Sep 23, 2017

Inscrutable left a comment

Inscrutable Sep 26, 2017

Inscrutable Sep 26, 2017

Inscrutable Sep 26, 2017

Inscrutable Sep 26, 2017

dualspiral Sep 26, 2017

ST-DDT commented Sep 26, 2017

Inscrutable Sep 27, 2017

ST-DDT Sep 27, 2017

ST-DDT commented Oct 4, 2017 •

edited


		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.

Document some bad practices that should be avoided: Fixes #541 #558

Document some bad practices that should be avoided: Fixes #541 #558

Conversation

ST-DDT commented Dec 19, 2016

Spongy commented Dec 19, 2016 • edited

Mumfrey commented Dec 19, 2016

Inscrutable left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ST-DDT commented Dec 20, 2016

ST-DDT commented Dec 20, 2016 • edited

MarkL4YG commented Feb 6, 2017

Inscrutable commented Jul 29, 2017

ST-DDT commented Sep 23, 2017

MarkL4YG commented Sep 23, 2017

ST-DDT commented Sep 23, 2017

MarkL4YG commented Sep 23, 2017

Inscrutable left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ST-DDT commented Sep 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ST-DDT commented Oct 4, 2017 • edited

Spongy commented Dec 19, 2016 •

edited

ST-DDT commented Dec 20, 2016 •

edited

ST-DDT commented Oct 4, 2017 •

edited