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.

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 getPreviousPosition npe #11621

Merged
merged 4 commits into from Aug 11, 2021

Conversation

gaozhangmin
Copy link
Contributor

@gaozhangmin gaozhangmin commented Aug 10, 2021

Fixes #11619

Motivation

There's a chance that headMap might be empty even if a empty check added. because remove would be executed in ledgers. after the empty check between line 3214 and line 3219 in following code snippet:

headMap is a view of ledgers

NavigableMap<Long, LedgerInfo> headMap = ledgers.headMap(position.getLedgerId(), false);
if (headMap.isEmpty()) {
// There is no previous ledger, return an invalid position in the current ledger
return PositionImpl.get(position.getLedgerId(), -1);
}
// We need to find the most recent non-empty ledger
for (long ledgerId : headMap.descendingKeySet()) {
LedgerInfo li = headMap.get(ledgerId);
if (li.getEntries() > 0) {
return PositionImpl.get(li.getLedgerId(), li.getEntries() - 1);
}
}
// in case there are only empty ledgers, we return a position in the first one
return PositionImpl.get(headMap.firstEntry().getKey(), -1);

Modifications

Add a null checker of headMap.firstEntry

Verifying this change
Make sure that the change passes the CI checks.
This change is a trivial rework / code cleanup without any test coverage.

Copy link
Contributor

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

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

This fix may fix the NPE of the original issue. But I thinks there're still possibilities for NPE because when we iterate over the headMap, there's a chance that headMap.get(ledgerId) returns null even if ledgerId is in its key set.
IMO, a better solution is make a copy of ledgers so the copied ledgers is constant during getPreviousPosition.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

LGTM good catch

unfortunately there is no good way to add a test

@hangc0276
Copy link
Contributor

move to 2.8.2

@hangc0276 hangc0276 added this to the 2.9.0 milestone Aug 11, 2021
@hangc0276
Copy link
Contributor

move back to 2.8.1

@hangc0276 hangc0276 merged commit 7f7eb9d into apache:master Aug 11, 2021
hangc0276 pushed a commit that referenced this pull request Aug 12, 2021
Fixes #11619

### Motivation
There's a chance that headMap might be empty even if a empty check added. because remove would be executed in ledgers. after the empty check between line 3214 and line 3219 in following code snippet:

headMap is a view of ledgers

pulsar/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java

Lines 3206 to 3222 in 31231d6
```
 NavigableMap<Long, LedgerInfo> headMap = ledgers.headMap(position.getLedgerId(), false);

 if (headMap.isEmpty()) {
     // There is no previous ledger, return an invalid position in the current ledger
     return PositionImpl.get(position.getLedgerId(), -1);
 }

 // We need to find the most recent non-empty ledger
 for (long ledgerId : headMap.descendingKeySet()) {
     LedgerInfo li = headMap.get(ledgerId);
     if (li.getEntries() > 0) {
         return PositionImpl.get(li.getLedgerId(), li.getEntries() - 1);
     }
 }

 // in case there are only empty ledgers, we return a position in the first one
 return PositionImpl.get(headMap.firstEntry().getKey(), -1);
```

### Modifications
Add a null checker of headMap.firstEntry

Verifying this change
Make sure that the change passes the CI checks.
This change is a trivial rework / code cleanup without any test coverage.

Co-authored-by: gavingaozhangmin <gavingaozhangmin@didiglobal.com>
(cherry picked from commit 7f7eb9d)
bharanic-dev pushed a commit to bharanic-dev/pulsar that referenced this pull request Mar 18, 2022
Fixes apache#11619

### Motivation
There's a chance that headMap might be empty even if a empty check added. because remove would be executed in ledgers. after the empty check between line 3214 and line 3219 in following code snippet:

headMap is a view of ledgers

pulsar/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java

Lines 3206 to 3222 in 31231d6
```
 NavigableMap<Long, LedgerInfo> headMap = ledgers.headMap(position.getLedgerId(), false); 
  
 if (headMap.isEmpty()) { 
     // There is no previous ledger, return an invalid position in the current ledger 
     return PositionImpl.get(position.getLedgerId(), -1); 
 } 
  
 // We need to find the most recent non-empty ledger 
 for (long ledgerId : headMap.descendingKeySet()) { 
     LedgerInfo li = headMap.get(ledgerId); 
     if (li.getEntries() > 0) { 
         return PositionImpl.get(li.getLedgerId(), li.getEntries() - 1); 
     } 
 } 
  
 // in case there are only empty ledgers, we return a position in the first one 
 return PositionImpl.get(headMap.firstEntry().getKey(), -1); 
```

### Modifications
Add a null checker of headMap.firstEntry

Verifying this change
Make sure that the change passes the CI checks.
This change is a trivial rework / code cleanup without any test coverage.

Co-authored-by: gavingaozhangmin <gavingaozhangmin@didiglobal.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-picked/branch-2.8 Archived: 2.8 is end of life release/2.8.1 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NPE when getPreviousPosition
5 participants