[fix][broker] fix prepareInitPoliciesCacheAsync in SystemTopicBasedTopicPoliciesService #24980

TakaHiR07 · 2025-11-14T09:59:48Z

Motivation

As shown in the issue, fix two problem: 1. cleanCacheAndCloseReader() executed twice cause concurrent error, which result in too many orphan reader remain in SystemTopicBasedTopicPoliciesService 2. double update in policyCacheInitMap cause recursive update error

Modifications

do cleanPoliciesCacheInitMap only once when throw exception
avoid double update in policyCacheInitMap. use putIfAbsent instead of computeIfAbsent. It is not appropriate to add so many operation into compute().
add two test, to simulate if throw exception in createReader, initPolicyCache, readMorePolicy of prepareInitPoliciesCacheAsync. By the way, it seems lack of unittest in SystemTopicBasedTopicPoliciesService.
"newReader()" remove some logic, it is confused when readCompletableFuture throw exception.
not remove cleanPoliciesCacheInitMap() in initPolicesCache() when closed.get()==true, since broker is closed, clean twice is ok.

There is one point should be consider in this pr

When use putIfAbsent, if too many getTopicPolicy() trigger prepareInitPoliciesCacheAsync, it would generate many empty completableFuture. Further more, we can use double check in the code to avoid this object gc.(the code would be ugly).

Besides, this case still exist: if failed to close reader in cleanCacheAndCloseReader(), this closing reader maybe have chance to reconnect and become orphan reader. This is not this pr's work.

Verifying this change

Make sure that the change passes the CI checks.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

Documentation

doc
doc-required
doc-not-needed
doc-complete

…ervice

lhotari

LGTM, thanks for the great work @TakaHiR07

codecov-commenter · 2025-11-14T11:14:13Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.29%. Comparing base (6fdb4b9) to head (33ae945).
⚠️ Report is 10 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #24980   +/-   ##
=========================================
  Coverage     74.29%   74.29%           
- Complexity    34026    34066   +40     
=========================================
  Files          1920     1920           
  Lines        150252   150252           
  Branches      17428    17428           
=========================================
+ Hits         111634   111636    +2     
- Misses        29706    29735   +29     
+ Partials       8912     8881   -31

Flag	Coverage Δ
inttests	`26.17% <75.00%> (-0.39%)`	⬇️
systests	`22.87% <67.85%> (-0.02%)`	⬇️
unittests	`73.84% <100.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
.../service/SystemTopicBasedTopicPoliciesService.java	`77.86% <100.00%> (+0.19%)`	⬆️

... and 78 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot

Pull Request Overview

This PR fixes issues in SystemTopicBasedTopicPoliciesService.prepareInitPoliciesCacheAsync related to duplicate cleanup execution and recursive update errors when exceptions occur during policy cache initialization.

Key Changes:

Replaced computeIfAbsent with putIfAbsent to avoid recursive update errors when modifying the map during computation
Consolidated exception handling to ensure cleanPoliciesCacheInitMap is called only once per exception
Removed redundant cleanup calls from initPolicesCache method to prevent double cleanup
Simplified reader creation logic in newReader by removing special exception handling that is now redundant

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
SystemTopicBasedTopicPoliciesService.java	Refactored `prepareInitPoliciesCacheAsync` to use `putIfAbsent` instead of `computeIfAbsent`, consolidated exception handling to prevent double cleanup, simplified `newReader` logic, and changed `cleanPoliciesCacheInitMap` visibility for testing
SystemTopicBasedTopicPoliciesServiceTest.java	Added two comprehensive test cases to verify correct cleanup behavior when exceptions occur during reader creation and policy cache initialization

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

...src/test/java/org/apache/pulsar/broker/service/SystemTopicBasedTopicPoliciesServiceTest.java

Technoboy- · 2025-11-19T04:05:38Z

Which logic could cause this issue ?

Request-1: policyCacheInitMap put future1
Request-1: create reader1
Request-1: readerCaches put reader1
reader1 read error
Request-1: first time cleanCacheAndCloseReader(), include:
        remove reader1 in readerCaches
        close reader1
        remove future1 in policyCacheInitMap

Request-2: policyCacheInitMap put future2
Request-1: second time cleanCacheAndCloseReader(), only remove future2 in policyCacheInitMap
Request-2: create reader2
Request-2: readerCaches put reader2

Request-3: policyCacheInitMap put future3
Request-3: create reader3
Request-3: readerCaches put reader3

Technoboy- · 2025-11-19T04:09:35Z

Which logic could cause this issue ?

Request-1: policyCacheInitMap put future1
Request-1: create reader1
Request-1: readerCaches put reader1
reader1 read error
Request-1: first time cleanCacheAndCloseReader(), include:
        remove reader1 in readerCaches
        close reader1
        remove future1 in policyCacheInitMap

Request-2: policyCacheInitMap put future2
Request-1: second time cleanCacheAndCloseReader(), only remove future2 in policyCacheInitMap
Request-2: create reader2
Request-2: readerCaches put reader2

Request-3: policyCacheInitMap put future3
Request-3: create reader3
Request-3: readerCaches put reader3

Is this bug existed in the 3.0.x , not the latest version ?

TakaHiR07 · 2025-11-19T04:18:55Z

Which logic could cause this issue ?

@Technoboy- restart broker with version-3.0.x. Restart broker-1, and after a few time restart broker-2. When load topic and getTopicPolicy on broker-1, the corresponding __change_event topic on broker-2 is unload.

I don't use the latest version. Maybe in latest version, this concurrent case is avoid by pr-24658. But it still catch the exception and cleanCacheAndPolicyMap twice, this is dangerous.

Technoboy- · 2025-11-19T04:38:36Z

Which logic could cause this issue ?

@Technoboy- restart broker with version-3.0.x. Restart broker-1, and after a few time restart broker-2. When load topic and getTopicPolicy on broker-1, the corresponding __change_event topic on broker-2 is unload.

I don't use the latest version. Maybe in latest version, this concurrent case is avoid by pr-24658. But it still catch the exception and cleanCacheAndPolicyMap twice, this is dangerous.

How could the latest code cause the issue ? I'm not understand

TakaHiR07 · 2025-11-19T06:46:11Z

How could the latest code cause the issue ? I'm not understand

You can see the code in branch-3.0. Latest code is a bit different, the concurrent case is found on branch-3.0

Technoboy- · 2025-11-19T08:55:46Z

How could the latest code cause the issue ? I'm not understand

You can see the code in branch-3.0. Latest code is a bit different, the concurrent case is found on branch-3.0

So it's better to fix it to branch-3.0, for the master branch, I don't think it's needed.

TakaHiR07 · 2025-11-19T09:22:47Z

So it's better to fix it to branch-3.0, for the master branch, I don't think it's needed.

@Technoboy- I think it is better to also fix in master branch. Since the current code in master branch is for improvement, not a true fix, and still have risk.

lhotari · 2025-12-10T14:56:24Z

As shown in the issue, fix two problem: 1. cleanCacheAndCloseReader() executed twice cause concurrent error, which result in too many orphan reader remain in SystemTopicBasedTopicPoliciesService 2. double update in policyCacheInitMap cause recursive update error

I think that this problem exists also in master branch and therefore merging this PR and cherry-picking it to maintenance branches makes sense.

lhotari · 2025-12-15T08:09:23Z

Depends on #24658 for branch-4.0 and branch-4.1

…picPoliciesService (#24980) Co-authored-by: fanjianye <fanjianye@bigo.sg> (cherry picked from commit 47b8d5d)

lhotari · 2025-12-16T10:19:03Z

Flaky test #25081, please take a look

…picPoliciesService (apache#24980) Co-authored-by: fanjianye <fanjianye@bigo.sg> (cherry picked from commit 47b8d5d) (cherry picked from commit 9ca5241)

fix prepareInitPoliciesCacheAsync() in SystemTopicBasedTopicPoliciesS…

09e9b0b

…ervice

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 14, 2025

TakaHiR07 mentioned this pull request Nov 14, 2025

[fix][broker][branch-3.0] fix prepareInitPoliciesCacheAsync in SystemTopicBasedTopicPoliciesService #24978

Merged

14 tasks

lhotari added the ready-to-test label Nov 14, 2025

lhotari approved these changes Nov 14, 2025

View reviewed changes

lhotari added release/4.0.9 release/4.1.3 labels Nov 14, 2025

lhotari requested review from Technoboy-, Copilot and nodece November 14, 2025 12:30

Copilot started reviewing on behalf of lhotari November 14, 2025 12:30 View session

lhotari requested review from BewareMyPower, coderzc and dao-jun November 14, 2025 12:31

Copilot finished reviewing on behalf of lhotari November 14, 2025 12:33

Copilot AI reviewed Nov 14, 2025

View reviewed changes

improve code in test

33ae945

dao-jun approved these changes Nov 18, 2025

View reviewed changes

lhotari merged commit 47b8d5d into apache:master Dec 10, 2025
52 checks passed

lhotari mentioned this pull request Dec 15, 2025

[improve][broker] Optimize Reader creation in TopicPoliciesService #24658

Merged

15 tasks

lhotari pushed a commit that referenced this pull request Dec 15, 2025

[fix][broker] fix prepareInitPoliciesCacheAsync in SystemTopicBasedTo…

80dacad

…picPoliciesService (#24980) Co-authored-by: fanjianye <fanjianye@bigo.sg> (cherry picked from commit 47b8d5d)

lhotari added the cherry-picked/branch-4.1 label Dec 15, 2025

lhotari pushed a commit that referenced this pull request Dec 15, 2025

[fix][broker] fix prepareInitPoliciesCacheAsync in SystemTopicBasedTo…

9ca5241

…picPoliciesService (#24980) Co-authored-by: fanjianye <fanjianye@bigo.sg> (cherry picked from commit 47b8d5d)

lhotari added the cherry-picked/branch-4.0 label Dec 15, 2025

lhotari mentioned this pull request Dec 16, 2025

Flaky-test: SystemTopicBasedTopicPoliciesServiceTest.testPrepareInitPoliciesCacheAsyncThrowExceptionAfterCreateReader #25081

Open

2 tasks

[fix][broker] fix prepareInitPoliciesCacheAsync in SystemTopicBasedTopicPoliciesService #24980

[fix][broker] fix prepareInitPoliciesCacheAsync in SystemTopicBasedTopicPoliciesService #24980

Conversation

TakaHiR07 commented Nov 14, 2025 • edited by lhotari Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Uh oh!

lhotari left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Technoboy- commented Nov 19, 2025

Uh oh!

Technoboy- commented Nov 19, 2025

Uh oh!

TakaHiR07 commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Technoboy- commented Nov 19, 2025

Uh oh!

TakaHiR07 commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Technoboy- commented Nov 19, 2025

Uh oh!

TakaHiR07 commented Nov 19, 2025

Uh oh!

lhotari commented Dec 10, 2025

Uh oh!

Uh oh!

lhotari commented Dec 15, 2025

Uh oh!

lhotari commented Dec 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

TakaHiR07 commented Nov 14, 2025 •

edited by lhotari

Loading

codecov-commenter commented Nov 14, 2025 •

edited

Loading

TakaHiR07 commented Nov 19, 2025 •

edited

Loading

TakaHiR07 commented Nov 19, 2025 •

edited

Loading