[fix][meta] Fix deadlock causes session notification not to work by mattisonchao · Pull Request #19754 · apache/pulsar

mattisonchao · 2023-03-08T16:54:47Z

Motivation

This is a namespace bundle double-owners problem. We found it in the memory dumps.

The memory dumps show that the notification thread has been blocked for a long time by the leader election deadlock, And many notifications are blocked in the executor queue. This causes we can't to revalidate the locks, and they are still thinking them working well.

For private reasons, I can't share the namespace bundle snapshot, but the blocked thread easily explains it.

^^ blocked thread

^^ executor queue

Modifications

Avoid putting the new task to single thread executor causes deadlock.

Verifying this change

Make sure that the change passes the CI checks.

Documentation

doc-not-needed

This reverts commit 1e4d638.

This reverts commit 7adf86f.

…wner." This reverts commit 542cc5a.

github-actions · 2023-03-08T16:55:25Z

@mattisonchao Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

mattisonchao · 2023-03-08T16:59:50Z

@lhotari would you mind taking a look? it looks like the problem introduced by #17401

I think it's a critical issue for our users who have enabled session listening.

mattisonchao · 2023-03-08T17:02:18Z

+        }).thenCompose(leaderElectionState -> {
            // make sure that the cache contains the current leader
            // so that getLeaderValueIfPresent works on all brokers
            cache.refresh(path);


I checked the logic. This method looks changed and no blocking call anymore. /cc @lhotari

mattisonchao · 2023-03-08T17:11:00Z

            cache.refresh(path);
            return cache.get(path)
                    .thenApply(__ -> leaderElectionState);
-        }, executor);


We can't use this single thread executor here because session notification method ( handleSessionNotification )also uses this executor to do blocking get. It will cause a deadlock.

codelipenghui

Nice catch!

And I think it should be a critical bug introduced to 2.8.5, 2.9.4, 2.10.3, 2.11.1.
IMO, we should notice it in the mailing list to clarify it. It will help users decide
whether to upgrade to 2.8.5, 2.9.4, 2.10.3, or 2.11.0. Or whether to downgrade
to 2.8.4, 2.9.3, 2.10.2.

As I know, 2.9.5, 2.10.4, and 2.11.1 is in the release process. We should ship this
fix to the subsequent patch releases. For 2.8.5, maybe we also need to plan 2.8.6.

For the solution that the PR introduced. Yes, I think it can fix the issue.
IMO, instead of changing the thread, can we change the handleSessionNotification
method to an async method? I mean just to avoid any cases that block the executor
specified to the LeaderElectionImpl. I think it is not expected, right?

A possible solution is when the session event arrives. We can just put the event
in a queue and process it asynchronously. Like processing the next event if the
future of the current event is done. So that we don't need to block any thread.

mattisonchao · 2023-03-09T01:54:14Z

For the solution that the PR introduced. Yes, I think it can fix the issue.
IMO, instead of changing the thread, can we change the handleSessionNotification
method to an async method? I mean just to avoid any cases that block the executor
specified to the LeaderElectionImpl. I think it is not expected, right?

Yes, that is what I thought, which may change much logic here. The async changes are just an improvement, not a bug fix. We can do it in the next PR. since the more complex changes will need more time to review.

We can just revert the part of #17401 to cherry-pick it to previous branches.

WDYT? @codelipenghui

codelipenghui · 2023-03-09T01:59:04Z

Yes, that is what I thought, which may change much logic here. The async changes are just an improvement, not a bug fix. We can do it in the next PR. since the more complex changes will need more time to review.

@mattisonchao Ok, sounds good to me. Let's revert the problem change to the old version, then find a better solution.

poorbarcode

LGTM

lordcheng10 · 2023-03-09T02:36:38Z

nice catch!

) ### Motivation This is a namespace bundle double-owners problem. We found it in the memory dumps. The memory dumps show that the notification thread has been blocked for a long time by the leader election deadlock, And many notifications are blocked in the executor queue. This causes we can't to revalidate the locks, and they are still thinking them working well. For private reasons, I can't share the namespace bundle snapshot, but the blocked thread easily explains it. <img width="1061" alt="image" src="https://user-images.githubusercontent.com/74767115/223670419-c4319f44-f1e1-4361-8c79-04c7f0dabe3c.png"> ^^ blocked thread <img width="949" alt="image" src="https://user-images.githubusercontent.com/74767115/223672000-fb4ce22c-6a45-4cb6-9d23-c36e3afbc93a.png"> ^^ executor queue ### Modifications - Avoid putting the new task to single thread executor causes deadlock. (cherry picked from commit cbd799f)

…ot to work (#19754) (#19768) Co-authored-by: Qiang Zhao <mattisonchao@apache.org> Master #19754 ### Motivation This is a namespace bundle double-owners problem. We found it in the memory dumps. The memory dumps show that the notification thread has been blocked for a long time by the leader election deadlock, And many notifications are blocked in the executor queue. This causes we can't to revalidate the locks, and they are still thinking them working well. For private reasons, I can't share the namespace bundle snapshot, but the blocked thread easily explains it. <img width="1061" alt="image" src="https://user-images.githubusercontent.com/74767115/223670419-c4319f44-f1e1-4361-8c79-04c7f0dabe3c.png"> ^^ blocked thread <img width="949" alt="image" src="https://user-images.githubusercontent.com/74767115/223672000-fb4ce22c-6a45-4cb6-9d23-c36e3afbc93a.png"> ^^ executor queue ### Modifications - Avoid putting the new task to single thread executor causes deadlock. (cherry picked from commit cbd799f)

) ### Motivation This is a namespace bundle double-owners problem. We found it in the memory dumps. The memory dumps show that the notification thread has been blocked for a long time by the leader election deadlock, And many notifications are blocked in the executor queue. This causes we can't to revalidate the locks, and they are still thinking them working well. For private reasons, I can't share the namespace bundle snapshot, but the blocked thread easily explains it. <img width="1061" alt="image" src="https://user-images.githubusercontent.com/74767115/223670419-c4319f44-f1e1-4361-8c79-04c7f0dabe3c.png"> ^^ blocked thread <img width="949" alt="image" src="https://user-images.githubusercontent.com/74767115/223672000-fb4ce22c-6a45-4cb6-9d23-c36e3afbc93a.png"> ^^ executor queue ### Modifications - Avoid putting the new task to single thread executor causes deadlock.

…ot to work (apache#19754) (apache#19768) Co-authored-by: Qiang Zhao <mattisonchao@apache.org> Master apache#19754 ### Motivation This is a namespace bundle double-owners problem. We found it in the memory dumps. The memory dumps show that the notification thread has been blocked for a long time by the leader election deadlock, And many notifications are blocked in the executor queue. This causes we can't to revalidate the locks, and they are still thinking them working well. For private reasons, I can't share the namespace bundle snapshot, but the blocked thread easily explains it. <img width="1061" alt="image" src="https://user-images.githubusercontent.com/74767115/223670419-c4319f44-f1e1-4361-8c79-04c7f0dabe3c.png"> ^^ blocked thread <img width="949" alt="image" src="https://user-images.githubusercontent.com/74767115/223672000-fb4ce22c-6a45-4cb6-9d23-c36e3afbc93a.png"> ^^ executor queue ### Modifications - Avoid putting the new task to single thread executor causes deadlock. (cherry picked from commit cbd799f) (cherry picked from commit 36794d9)

…che#19754) ### Motivation This is a namespace bundle double-owners problem. We found it in the memory dumps. The memory dumps show that the notification thread has been blocked for a long time by the leader election deadlock, And many notifications are blocked in the executor queue. This causes we can't to revalidate the locks, and they are still thinking them working well. For private reasons, I can't share the namespace bundle snapshot, but the blocked thread easily explains it. <img width="1061" alt="image" src="https://user-images.githubusercontent.com/74767115/223670419-c4319f44-f1e1-4361-8c79-04c7f0dabe3c.png"> ^^ blocked thread <img width="949" alt="image" src="https://user-images.githubusercontent.com/74767115/223672000-fb4ce22c-6a45-4cb6-9d23-c36e3afbc93a.png"> ^^ executor queue ### Modifications - Avoid putting the new task to single thread executor causes deadlock. (cherry picked from commit cbd799f)

mattisonchao added 7 commits March 8, 2023 16:47

[fix][metadata] Fix notification thread block causes double owner.

542cc5a

Fix the doc

7adf86f

Fix checkstyle

1e4d638

[fix][meta] Fix deadlock causes session notification is not to work

a098e17

Revert "Fix checkstyle"

0c53166

This reverts commit 1e4d638.

Revert "Fix the doc"

a395003

This reverts commit 7adf86f.

Revert "[fix][metadata] Fix notification thread block causes double o…

a377e6f

…wner." This reverts commit 542cc5a.

github-actions Bot added the doc-label-missing label Mar 8, 2023

mattisonchao self-assigned this Mar 8, 2023

mattisonchao requested a review from lhotari March 8, 2023 16:58

github-actions Bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Mar 8, 2023

mattisonchao requested review from codelipenghui and merlimat March 8, 2023 16:58

mattisonchao added the ready-to-test label Mar 8, 2023

mattisonchao added this to the 3.0.0 milestone Mar 8, 2023

mattisonchao added release/2.11.1 release/2.10.5 release/2.9.6 labels Mar 8, 2023

mattisonchao commented Mar 8, 2023

View reviewed changes

mattisonchao closed this Mar 8, 2023

mattisonchao reopened this Mar 8, 2023

Fix checkstyle

80d0613

mattisonchao commented Mar 8, 2023

View reviewed changes

eolivelli reviewed Mar 8, 2023

View reviewed changes

Comment thread ...-metadata/src/main/java/org/apache/pulsar/metadata/coordination/impl/LeaderElectionImpl.java

codelipenghui reviewed Mar 9, 2023

View reviewed changes

Comment thread ...adata/src/test/java/org/apache/pulsar/metadata/coordination/impl/LeaderElectionImplTest.java

codelipenghui approved these changes Mar 9, 2023

View reviewed changes

poorbarcode approved these changes Mar 9, 2023

View reviewed changes

liangyepianzhou approved these changes Mar 9, 2023

View reviewed changes

mattisonchao changed the title ~~[fix][meta] Fix deadlock causes session notification is not to work~~ [fix][meta] Fix deadlock causes session notification not to work Mar 9, 2023

Technoboy- approved these changes Mar 9, 2023

View reviewed changes

liangyepianzhou merged commit cbd799f into apache:master Mar 9, 2023

mattisonchao added release/2.10.4 release/blocker Indicate the PR or issue that should block the release until it gets resolved release/2.9.5 and removed release/2.10.5 release/2.9.6 labels Mar 9, 2023

coderzc added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Mar 9, 2023

mattisonchao deleted the fix/double/leader-2 branch March 9, 2023 03:49

lhotari mentioned this pull request Mar 9, 2023

Flaky-test: ZKSessionTest.testReacquireLeadershipAfterSessionLost #11032

Closed

liangyepianzhou mentioned this pull request Mar 9, 2023

[cherry-pick][branch-2.10] Fix deadlock causes session notification not to work (#19754) #19768

Merged

15 tasks

liangyepianzhou added the cherry-picked/branch-2.10 label Mar 10, 2023

Technoboy- added the cherry-picked/branch-2.11 label Mar 10, 2023

mattisonchao mentioned this pull request Mar 21, 2023

[improve][meta] Make session notification to be async. #19869

Merged

5 tasks

mattisonchao removed the release/blocker Indicate the PR or issue that should block the release until it gets resolved label Mar 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[fix][meta] Fix deadlock causes session notification not to work - #19754

[fix][meta] Fix deadlock causes session notification not to work#19754
liangyepianzhou merged 8 commits into
apache:masterfrom
mattisonchao:fix/double/leader-2

mattisonchao commented Mar 8, 2023 •

edited

Loading

Uh oh!

github-actions Bot commented Mar 8, 2023

Uh oh!

mattisonchao commented Mar 8, 2023 •

edited

Loading

Uh oh!

mattisonchao Mar 8, 2023 •

edited

Loading

Uh oh!

mattisonchao Mar 8, 2023 •

edited

Loading

Uh oh!

Uh oh!

codelipenghui left a comment •

edited

Loading

Uh oh!

Uh oh!

mattisonchao commented Mar 9, 2023 •

edited

Loading

Uh oh!

codelipenghui commented Mar 9, 2023

Uh oh!

poorbarcode left a comment

Uh oh!

lordcheng10 commented Mar 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Uh oh!

Conversation

mattisonchao commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

Documentation

Uh oh!

github-actions Bot commented Mar 8, 2023

Uh oh!

mattisonchao commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattisonchao Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattisonchao Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codelipenghui left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattisonchao commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codelipenghui commented Mar 9, 2023

Uh oh!

poorbarcode left a comment

Choose a reason for hiding this comment

Uh oh!

lordcheng10 commented Mar 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

mattisonchao commented Mar 8, 2023 •

edited

Loading

mattisonchao commented Mar 8, 2023 •

edited

Loading

mattisonchao Mar 8, 2023 •

edited

Loading

mattisonchao Mar 8, 2023 •

edited

Loading

codelipenghui left a comment •

edited

Loading

mattisonchao commented Mar 9, 2023 •

edited

Loading