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

Fix #609 #628

Merged
merged 13 commits into from Dec 24, 2017

Conversation

Projects
None yet
2 participants
@Pr0methean
Contributor

Pr0methean commented Dec 20, 2017

This fix uses a lock that is acquired by teleport(), pulse() and spawnAt(). We can add it in more places if needed.

Pr0methean added some commits Dec 20, 2017

Fix #609
This fix uses a lock that is acquired by teleport(), pulse() and
spawnAt().
@smartboyathome

This comment has been minimized.

Contributor

smartboyathome commented Dec 20, 2017

The world lock seems like its doing too much. It's guarding a couple cases where the entity modifies GlowWorld, but at the same time its guarding the problems with knownEntities, which doesn't seem to touch the world, nor the world touch knownEntities? Since the initial bug was around knownEntities, it seems a ReadWriteLock would be in order here, with canSeeEntity() acquiring a read lock while hidePlayer(), pulse(), and respawn() acquire the write lock.

Or am I missing something here?

@Pr0methean

This comment has been minimized.

Contributor

Pr0methean commented Dec 20, 2017

Switched to a readWriteLock. respawn() will also acquire the write lock, since it too can update knownEntities. In other words, worldLock guards both world and knownEntities.

Pr0methean added some commits Dec 20, 2017

@smartboyathome

This comment has been minimized.

Contributor

smartboyathome commented Dec 20, 2017

I still don't understand why you are (re-)using this lock for the .unregister() and .register() calls. I didn't see it accessing the knownEntities collection at any point.

@Pr0methean

This comment has been minimized.

Contributor

Pr0methean commented Dec 20, 2017

To ensure that when we register with the destination world, a spawn or another teleport hasn't moved us outside of that world (which may have something to do with bug #630 ).

@smartboyathome

This comment has been minimized.

Contributor

smartboyathome commented Dec 20, 2017

I think then that there should be a second lock for that case. Over reliance on a single lock for two different use cases can cause confusion as well as performance problems.

@Pr0methean

This comment has been minimized.

Contributor

Pr0methean commented Dec 20, 2017

But using two separate locks will mean world and knownEntities can be out of sync, with IDs being misinterpreted because they're from a different world. @momothereal, what do you think?

BTW, I'm now pretty sure #609 and #630 are related, since when I tested this PR, I only got Nether chunks in the Overworld, whereas I'd previously gotten End chunks as well.

@Pr0methean Pr0methean changed the title from Fix #609 to Fix #609, #630 Dec 21, 2017

@Pr0methean Pr0methean changed the title from Fix #609, #630 to Fix #609 Dec 21, 2017

@Pr0methean Pr0methean referenced this pull request Dec 21, 2017

Closed

Mixed worlds in dev #630

@smartboyathome

This comment has been minimized.

Contributor

smartboyathome commented Dec 21, 2017

So, I did some deep diving into this code, and this doesn't appear to be operating quite how you expect it to. If you want to keep both collections in sync, you'll need to move the lock up to where both items are being manipulated, such as the GlowPlayer.respawn() method. What'll happen right now, taking the aforementioned method as an example, is:

  1. Acquire worldLock.WriteLock 1
  2. Manipulate knownEntities
  3. Release worldLock.WriteLock 1
  4. Call spawnat()
  5. Acquire worldLock.WriteLock 2
  6. EntityManager.register()
  7. release worldLock.WriteLock 2

As you can see, the lock was acquired and twice (two separate instances at that), and between the first instance being released 3 and the second instance being acquired in 5, the GlowPlayer could be manipulated, making the two go out of sync. The fix to this is to make sure you never release the lock while working on either of these operations.

@smartboyathome smartboyathome self-requested a review Dec 21, 2017

@Pr0methean

This comment has been minimized.

Contributor

Pr0methean commented Dec 21, 2017

Thanks! I've fixed that case. As well, respawn() will also hold the lock while querying the player's world for a default spawn location (although I think that affects only modded servers, since players who die in the Nether or End, or who fall into a Nether/End portal while dead, would respawn in the Overworld in vanilla).

Pr0methean added some commits Dec 21, 2017

Bug fix
CME was still occurring due to the asynchrony of adding entities

@Pr0methean Pr0methean merged commit 04f7469 into GlowstoneMC:dev Dec 24, 2017

2 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
license/cla Contributor License Agreement is signed.
Details

@Pr0methean Pr0methean deleted the Pr0methean:concurrency2 branch Dec 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment