Skip to content

[Java] fix shutdown lock typo#11410

Merged
kfstorm merged 2 commits intoray-project:masterfrom
antgroup:java_shutdownlock_fix
Oct 20, 2020
Merged

[Java] fix shutdown lock typo#11410
kfstorm merged 2 commits intoray-project:masterfrom
antgroup:java_shutdownlock_fix

Conversation

@kfstorm
Copy link
Copy Markdown
Member

@kfstorm kfstorm commented Oct 15, 2020

Why are these changes needed?

Related issue number

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@Override
public void shutdown() {
Lock writeLock = shutdownLock.readLock();
Lock writeLock = shutdownLock.writeLock();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two questions:

  1. Will shutdown be called by multithreading?
  2. Why there is no problem with UT?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. shutdown won't be called concurrently, but the lock is also used in NativeObjectStore. When an object is GCed, the object will be unregistered from core worker. Since GC runs in a separate thread, we need to make sure that core worker is available when NativeObjectStore is accessing core worker in the GC thread.

  2. There was some occasional failures in ReferenceCountingTest, but since we've added retry recently (like Python tests), it's unlikely to fail 3 times in a test session.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shutdown won't be called concurrently, but the lock is also used in NativeObjectStore. When an object is GCed, the object will be unregistered from core worker. Since GC runs in a separate thread, we need to make sure that core worker is available when NativeObjectStore is accessing core worker in the GC thread.

How about add it to the code comment?

@kfstorm kfstorm merged commit cbc5dac into ray-project:master Oct 20, 2020
@kfstorm kfstorm deleted the java_shutdownlock_fix branch October 20, 2020 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants