duongkame commented on PR #1045:
URL: https://github.com/apache/ratis/pull/1045#issuecomment-1982036377
> @duongkame , thanks for working not this! We should talk about the
contract of the new `RaftLog.getWithRef`. Are `retain()`/`release()` required?
If not, when is safe to use it?
>
> Currently, `SegmentedRaftLog` may release the returned `ref` and causing
an exception as below:
>
> 1. SegmentedRaftLog loads an entry, creates `ref` and calls `retain()` in
order to save it to cache.
> 2. SegmentedRaftLog.getWithRef(..) returns `ref` to a caller.
> 3. SegmentedRaftLog cache evicts the entry, the reference count becomes 0
and `ref` is completely released.
> 4. The caller use `ref` by calling `get()` or `retain()`. Then, it will
get an exception.
Yeah, that's a good point. I had same question but wasn't sure if the log
entry can be evicted shortly after being called with `get` from
SegmentedRaftLog. If theoretically eviction can happen immediately, calling
`retain()` after `getWithRef()` can help either as there's no coordination
between the SegmentedRaftLog internal state and the `ReferenceCountedObject`
after it is returned.
To properly prevent this error, the `retain()` needs to be called in the
readLock before `getWithRef` returns to prevent it from being cleaned up. It
means a new API that looks like
```
# SegmentedRaftLog API
void readLogEntry(long indec, Consumer<LogEntryProto> consumer) {
try (AutoCloseableLock readLock = readLock()) {
segment = cache.getSegment(index);
final ReferenceCountedObject<LogEntryProto> entry =
segment.getEntryFromCache(record.getTermIndex());
LogEntryProto log = entry.retain();
try {
consumer.consume(log);
} finally {
entry.release();
}
}
}
```
--
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]
