lhotari commented on code in PR #23903:
URL: https://github.com/apache/pulsar/pull/23903#discussion_r1932941546
##########
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:
> It seems like this value is a mutable obj. How do we guarantee this
value's state remains the same in this method?
There's no such guarantees here and that's not necessary. My previous answer
explains that "use after release" issues are prevents and there's a guarantee
that at this time the check passes. As explained in the previous answer, it
would be unrealistic to expect that this would prevent application double
release bugs.
--
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]
