LinkedList: Change assignment order in TLink.remove

[nittka]

In TVTower, I experience sporadic segmentation faults. Some of them could be traced to null access in the "contains" method of TLinkedList where the link value is compared to the parameter object, but the link value is null. This probably happens because several threads access the same list, concurrently changing and iterating over it.

With this change, the link's value is set to null only after the links of the neighbour nodes have been updated. This reduces the chance of a thread iterating a list currently being modified to look at a link whose value was set to null.

[HurryStarfish]

TLists are not thread-safe, and reordering these lines would not change that.
This seems like an issue that must be fixed in TVTower; multithreaded access to a TList should be protected by a lock.

[GWRon]

That change does not make it threadsafe...but less likely to run into a null access. So it just minimizes risks of that...nothing more.

[HurryStarfish]

Yeah, it could reduce the risk of that particular crash. But that might actually do more harm than good, because the problem would still there, just harder to notice.

[GWRon]

It won't be worse than before - as the issue is already not overly likely to happen. So I do not see it as "more harm than good". Crashes can still happen, in both cases it is rare to experience it (so hard to spot that there is an issue). So this PR just allows certain scenarios to become kinda threadsafe.

[HurryStarfish]

Well, "kinda thread-safe" still isn't thread-safe, and the more rarely these issues occur, the harder they become to find and fix.
But either way, changing the order of these instructions would not even really avoid this particular crash. Even if the assignments are written in a particular order, another thread might still see them happen in a different order. The wikipedia page on memory barriers has an example and explanation on this if you're interested.

[GWRon]

If execution orders are not guaranteed than switching lines does indeed make not much sense.

[nittka]

Do you agree that it would be better style (conceptually) to first change the links and then unassign the value? Even if the execution order is not guaranteed, I argue that it would be the correct order which should be reflected in the code.

If you consider accepting the PR, I will adapt the commit coment (making it sound less like introducing thread-safety, which I did not want to imply). Otherwise feel free to close the PR.