lhotari commented on code in PR #23903:
URL: https://github.com/apache/pulsar/pull/23903#discussion_r1932937963
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/util/RangeCache.java:
##########
@@ -236,34 +272,45 @@ public boolean exists(Key key) {
* The caller is responsible for releasing the reference.
*/
public Value get(Key key) {
- return getValue(key, entries.get(key));
+ return getValueFromWrapper(key, entries.get(key));
}
- private Value getValue(Key key, EntryWrapper<Key, Value> valueWrapper) {
+ private Value getValueFromWrapper(Key key, EntryWrapper<Key, Value>
valueWrapper) {
if (valueWrapper == null) {
return null;
} else {
Value value = valueWrapper.getValue(key);
- if (value == null) {
- // the wrapper has been recycled and contains another key
- return null;
- }
- try {
- value.retain();
- } catch (IllegalReferenceCountException e) {
- // Value was already deallocated
- return null;
- }
- // check that the value matches the key and that there's at least
2 references to it since
- // the cache should be holding one reference and a new reference
was just added in this method
- if (value.refCnt() > 1 && value.matchesKey(key)) {
- return value;
- } else {
- // Value or IdentityWrapper was recycled and already contains
another value
- // release the reference added in this method
- value.release();
- return null;
- }
+ return getRetainedValueMatchingKey(key, value);
+ }
+ }
+
+ private Value getValueMatchingEntry(Map.Entry<Key, EntryWrapper<Key,
Value>> entry) {
+ Value valueMatchingEntry =
EntryWrapper.getValueMatchingMapEntry(entry);
+ return getRetainedValueMatchingKey(entry.getKey(), valueMatchingEntry);
+ }
+
+ // validates that the value matches the key and that the value has not
been recycled
+ // which are possible due to the lack of exclusive locks in the cache and
the use of reference counted objects
+ private Value getRetainedValueMatchingKey(Key key, Value value) {
+ if (value == null) {
+ // the wrapper has been recycled and contains another key
+ return null;
+ }
+ try {
+ value.retain();
+ } catch (IllegalReferenceCountException e) {
+ // Value was already deallocated
+ return null;
+ }
+ // check that the value matches the key and that there's at least 2
references to it since
+ // the cache should be holding one reference and a new reference was
just added in this method
+ if (value.refCnt() > 1 && value.matchesKey(key)) {
+ return value;
Review Comment:
> What if this value is recycled after `if (value.refCnt() > 1 &&
value.matchesKey(key)) {` has a different value?
There's a call to `value.retain();` a few lines earlier. That is expected to
ensure that there's a reference to the value which prevents recycling. The
prevents "use after release" issues since it validates that the value matches
the key after the reference count has been incremented. Obviously it won't
prevent issues if there's a bug in application reference counting where the
application does a double release. That would be an unrealistic expectation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
