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.

Sign up for GitHub

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

Jump to bottom

Change synchronized on FileSystemContext to avoid deadlock #16219

Merged
merged 2 commits into from
Oct 9, 2022
Merged

Change synchronized on FileSystemContext to avoid deadlock #16219

merged 2 commits into from
Oct 9, 2022

Conversation

elega
Copy link
Contributor

@elega elega commented Sep 20, 2022

What changes are proposed in this pull request?

Please outline the changes and how this PR fixes the issue.

Why are the changes needed?

Please clarify why the changes are needed. For instance,

  1. If you propose a new API, clarify the use case for a new API.
  2. If you fix a bug, describe the bug.

Does this PR introduce any user facing changes?

Please list the user-facing changes introduced by your change, including

  1. change in user-facing APIs
  2. addition or removal of property keys
  3. webui

@elega elega force-pushed the yimin/remove-sync branch 2 times, most recently from dfb54db to 5e18049 Compare September 22, 2022 10:56
jiacheliu3
jiacheliu3 reviewed Sep 23, 2022
View reviewed changes
core/client/fs/src/main/java/alluxio/client/file/FileSystemContext.java
public List<BlockWorkerInfo> getCachedWorkers() throws IOException {
synchronized (mWorkerInfoList) {
if (mWorkerInfoList.get() == null || mWorkerInfoList.get().isEmpty()
|| mWorkerRefreshPolicy.attempt()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to worry about the thread safety of this mWorkerRefreshPolicy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated guarded by

@elega elega assigned jiacheliu3 and unassigned tcrain Sep 23, 2022
@elega elega force-pushed the yimin/remove-sync branch from c40edf1 to b01cbe4 Compare October 8, 2022 08:11
jiacheliu3
jiacheliu3 approved these changes Oct 9, 2022
View reviewed changes
Copy link
Contributor

@jiacheliu3 jiacheliu3 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@jiacheliu3 jiacheliu3 changed the title Resolve deadlocks in FileSystemContext by removing 'synchronized' Change synchronized on FileSystemContext.getCachedWorkers to avoid deadlock Oct 9, 2022
@alluxio-bot
Copy link
Contributor

Automated checks report:

  • Commits associated with Github account: PASS
  • PR title follows the conventions: FAIL
    • The title of the PR does not pass all the checks. Please fix the following issues:
      • Title is too long (75 characters). Must be at most 72 characters.

Some checks failed. Please fix the reported issues and reply 'alluxio-bot, check this please' to re-run checks.

@jiacheliu3 jiacheliu3 changed the title Change synchronized on FileSystemContext.getCachedWorkers to avoid deadlock Change synchronized on FileSystemContext to avoid deadlock Oct 9, 2022
@alluxio-bot
Copy link
Contributor

Automated checks report:

  • Commits associated with Github account: PASS
  • PR title follows the conventions: PASS

All checks passed!

@jiacheliu3
Copy link
Contributor

alluxio-bot, merge this please

@alluxio-bot alluxio-bot merged commit bec1062 into Alluxio:master Oct 9, 2022
@elega elega mentioned this pull request Oct 9, 2022
Open
jja725 pushed a commit to jja725/alluxio that referenced this pull request Jan 27, 2023
@elega @jja725
Change synchronized on FileSystemContext to avoid deadlock
2c18fee 
### What changes are proposed in this pull request?

Please outline the changes and how this PR fixes the issue.

### Why are the changes needed?

Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, describe the bug.

### Does this PR introduce any user facing changes?

Please list the user-facing changes introduced by your change, including
1. change in user-facing APIs
2. addition or removal of property keys
3. webui

pr-link: Alluxio#16219
change-id: cid-f6e6af4556026e122b2e672fad6ab87a0a9f507e
bzheng888 pushed a commit to bzheng888/alluxio that referenced this pull request Mar 8, 2023
@elega @maobaolong
Change synchronized on FileSystemContext to avoid deadlock
67df8a0 
### What changes are proposed in this pull request?

Please outline the changes and how this PR fixes the issue.

### Why are the changes needed?

Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, describe the bug.

### Does this PR introduce any user facing changes?

Please list the user-facing changes introduced by your change, including
1. change in user-facing APIs
2. addition or removal of property keys
3. webui

pr-link: Alluxio#16219
change-id: cid-f6e6af4556026e122b2e672fad6ab87a0a9f507e
@yuzhu
Copy link
Contributor

yuzhu commented Nov 27, 2024

alluxio-bot, cherry-pick this to branch-2.8 please

alluxio-bot pushed a commit that referenced this pull request Nov 27, 2024
@elega @alluxio-bot
Change synchronized on FileSystemContext to avoid deadlock
b9e70d2 
### What changes are proposed in this pull request?

Please outline the changes and how this PR fixes the issue.

### Why are the changes needed?

Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, describe the bug.

### Does this PR introduce any user facing changes?

Please list the user-facing changes introduced by your change, including
1. change in user-facing APIs
2. addition or removal of property keys
3. webui

pr-link: #16219
change-id: cid-f6e6af4556026e122b2e672fad6ab87a0a9f507e
@alluxio-bot
Copy link
Contributor

Auto cherry-pick to branch branch-2.8 successfully opened PR: #18711

@alluxio-bot alluxio-bot mentioned this pull request Nov 27, 2024
Open
yuzhu pushed a commit to yuzhu/alluxio that referenced this pull request Nov 27, 2024
@elega @yuzhu
Change synchronized on FileSystemContext to avoid deadlock
36f7e7e 
### What changes are proposed in this pull request?

Please outline the changes and how this PR fixes the issue.

### Why are the changes needed?

Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, describe the bug.

### Does this PR introduce any user facing changes?

Please list the user-facing changes introduced by your change, including
1. change in user-facing APIs
2. addition or removal of property keys
3. webui

pr-link: Alluxio#16219
change-id: cid-f6e6af4556026e122b2e672fad6ab87a0a9f507e
@yuzhu yuzhu mentioned this pull request Nov 27, 2024
Merged
Xenorith pushed a commit to yuzhu/alluxio that referenced this pull request Nov 27, 2024
@elega @Xenorith
Change synchronized on FileSystemContext to avoid deadlock
7ab4570 
### What changes are proposed in this pull request?

Please outline the changes and how this PR fixes the issue.

### Why are the changes needed?

Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, describe the bug.

### Does this PR introduce any user facing changes?

Please list the user-facing changes introduced by your change, including
1. change in user-facing APIs
2. addition or removal of property keys
3. webui

pr-link: Alluxio#16219
change-id: cid-f6e6af4556026e122b2e672fad6ab87a0a9f507e
Xenorith pushed a commit to yuzhu/alluxio that referenced this pull request Nov 27, 2024
@elega @Xenorith
Change synchronized on FileSystemContext to avoid deadlock
152b270 
### What changes are proposed in this pull request?

Please outline the changes and how this PR fixes the issue.

### Why are the changes needed?

Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, describe the bug.

### Does this PR introduce any user facing changes?

Please list the user-facing changes introduced by your change, including
1. change in user-facing APIs
2. addition or removal of property keys
3. webui

pr-link: Alluxio#16219
change-id: cid-f6e6af4556026e122b2e672fad6ab87a0a9f507e
alluxio-bot pushed a commit that referenced this pull request Nov 27, 2024
@yuzhu
Change synchronized on FileSystemContext to avoid deadlock
3963813 
Cherry-pick of existing commit.
orig-pr: #16219
orig-commit: bec1062
orig-commit-author: elega <445092967@qq.com>

			pr-link: #18712
			change-id: cid-f6e6af4556026e122b2e672fad6ab87a0a9f507e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jiacheliu3 jiacheliu3 jiacheliu3 approved these changes

Assignees

@jiacheliu3 jiacheliu3

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@elega @alluxio-bot @jiacheliu3 @yuzhu @tcrain