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

World thread-safety #507

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

@SquidDev
Copy link
Contributor

@SquidDev SquidDev commented Jan 16, 2018

Whilst the initial purpose of this was to fix world-thread safety, this PR has rather devolved into a general bugfix PR as it's easier than creating several inter-dependent PRs. Sorry.


This is an attempt at ensuring peripherals/other Lua calls do not access the world from the Lua thread. I'm going to be honest, none of the fixes are especially elegant. I'm expecting several of the fixes to require some level of polling, which is a pain. Currently most peripherals implement ITickable so it's not the end of the world, but it'd be nice to reduce the number which do in the future.

Current progress of the PR:

  • Cache direction of modems. (see #410).
  • Cache turtle family. (see SquidDev-CC/CCTweaks#113).
  • Cache monitor's terminal and peripheral type.
  • There's technically a race conditions in disk drives and the corresponding IMedia provider but I've been unable to actually cause it, so goodness knows.
  • Probably lots of other things I haven't come across yet.

I'd really appreciate any testing people could do, as there may be subtle differences in behaviour. Similarly, if people are aware of any other thread-unsafe accesses, do tell.

It's worth noting that some of these changes will probably become redundant in the 1.13 update as we'll probably want to split peripherals into their own separate classes. I still think it's worth getting this in before that though.

SquidDev added 2 commits Jan 16, 2018
This ensures the world is not accessed from another thread.

Closes dan200#410
This means one can call .getFamily() in a thread-safe manner, ensuring
turtle.getFuelLimit() does not cause issues. As we use a specialist
TE class for each family this does not require any specialist caching.
@Lupus590
Copy link
Contributor

@Lupus590 Lupus590 commented Jan 16, 2018

it's not the end of the world

If it causes a save corruption then, arguably, it is the end of that world. 🤣

I'll show myself off out...

@SquidDev SquidDev changed the title [WIP] World thread-safety World thread-safety Feb 14, 2018
@SquidDev
Copy link
Contributor Author

@SquidDev SquidDev commented Feb 14, 2018

As you may be able to tell, the monitor changes are slightly larger than anticipated. As the potential for breakage is larger, I'd recommend some serious testing before merging. We're currently using this on SwitchCraft, which caught a lot of the issues before they ended up in the PR.

If anyone's interested in helping test, please download from the CI server.

@SquidDev SquidDev force-pushed the feature/world-thread-safety branch 4 times, most recently from 62ff0cc to 5e8b4c7 Feb 21, 2018
SquidDev added 2 commits Feb 22, 2018
This restructures monitor in order to make it thread-safe: namely
removing any world interaction from the computer thread.

Instead of each monitor having their own terminal, resize flag, etc...
we use a monitor "multiblock" object. This is constructed on the origin
monitor and propagated to other monitors when required.

We attempt to construct the multiblock object (and so the corresponding
terminal) as lazily as posible. Consequently, we only create the
terminal when fetching the peripheral (not when attaching, as that is
done on the computer thread).

If a monitor is resized (say due to placing/breaking a monitor) then we
will invalidate all references to the multiblock object, construct a new
one if required, and propagate it to all component monitors.

This commit also fixes several instances of glLists not being deleted
after use. It is not a comprehensive fix, but that is outside the scope
of this commit.
Whilst this may add some performance overhead to terminal interactions,
it does ensure resizing terminals does not result in null pointer or out
of bounds exceptions.
@SquidDev SquidDev force-pushed the feature/world-thread-safety branch 2 times, most recently from 52e5aaa to 97c5a7c Feb 24, 2018
@SquidDev
Copy link
Contributor Author

@SquidDev SquidDev commented Feb 24, 2018

Whilst the last commits aren't strictly world/computer threading issues, but they fix various aspects about monitors and it was easier to bundle them here instead. I'm happy to revert them if inconvenient.

There's some other fixes I'd like to add at some point, but I'd like to confirm they work and are needed first.

SquidDev added 2 commits Mar 29, 2018
Shader mods may perform multiple passes when rendering a tile, so
monitors will be drawn transparently on later passes. In order to
prevent this we allow drawing the a single tile multiple times in a
tick.

See dan200#534
Shaders appear to ignore all the other subtle (and not-so-subtle) hints
we drop that monitors shouldn't be rendered with shadows. This solution
isn't optimal, as monitors may still be tinted due to sunlight, but
there is nothing we can do about that.

Many thanks to ferreusveritas for their help in diagnosing, fixing and
testing this issue.

Closes dan200#534
@thatcraniumguy
Copy link

@thatcraniumguy thatcraniumguy commented May 23, 2018

Can one of the admins verify this patch?

@SquidDev
Copy link
Contributor Author

@SquidDev SquidDev commented May 23, 2018

For what it's worth, I've been running a fork with almost all of my PRs merged on a relatively popular server and not seen any issues (it's actually more stable thanks to this pr).

That being said, there's not really much point bumping PRs. Dan merges stuff when he has time. While it's a little infuriating seeing stuff just sit there, I don't think it's fair to keep pestering Dan either.

@thatcraniumguy
Copy link

@thatcraniumguy thatcraniumguy commented May 23, 2018

I don't remember adding this comment... I think my Jenkins server pulled them all and auto-commented. My bad.

@SquidDev
Copy link
Contributor Author

@SquidDev SquidDev commented May 23, 2018

Ahhh, that explains a lot. I was rather confused when I woke up this morning and there were ~30 PRs with the same comment!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants