wernerdv commented on code in PR #13554:
URL: https://github.com/apache/ignite/pull/13554#discussion_r4004817279
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/freelist/AbstractFreeList.java:
##########
@@ -701,10 +713,29 @@ private int writeWholePages(T row, IoStatisticsHolder
statHolder) throws IgniteC
* @throws IgniteCheckedException If failed.
*/
private int writeSinglePage(T row, int written, IoStatisticsHolder
statHolder) throws IgniteCheckedException {
+ // TOCTOU closure: the size-aware reserve (ensureFreeSpaceForInsert,
invoked from RowStore.addRow/addRows
+ // before this write) accumulates enough real empty pages but does not
pin them to this thread - a concurrent
+ // writer can consume them between the reserve and this allocation.
When the free list cannot hand out a page,
+ // re-reserve on the remaining size and retry before allocating a
brand-new page; otherwise the race surfaces
+ // as a raw IgniteOutOfMemoryException (wrapped into
CorruptedFreeListException in the batch path).
+ //
+ // The re-reserve is an inline demand-eviction: reached from the
BPlusTree.invoke row-creation closure, it may
+ // re-entrantly remove other entries from the same data tree. That is
safe because the closure runs with no
+ // data-tree page locks held (page read lock released before it runs,
leaf write lock taken after), and the
+ // outer operation revalidates via the page tag / triangle / removeId
protocols. The key being written is
+ // skipped (its entry lock is held, so tryLock fails for it), so there
is no self-eviction or lock-ordering
+ // deadlock; like the initial reserve, the re-reserve throws OOM if
the row genuinely cannot fit.
AbstractDataPageIO initIo = null;
long pageId = takePage(row.size() - written, row, statHolder);
+ if (pageId == 0L) {
+ if (dbMgr != null)
+ dbMgr.ensureFreeSpaceForInsert(dataRegion, row.size() -
written);
+
+ pageId = takePage(row.size() - written, row, statHolder);
+ }
+
if (pageId == 0L) {
pageId = allocateDataPage(row.partition());
Review Comment:
Valid.
insertDataRows had the same allocateDataPage OOM-firing fallback without the
lazy re-reserve, and since IgniteOutOfMemoryException is a RuntimeException,
both methods mislabelled it as CorruptedFreeListException.
Made the batch trailing-fragment path symmetric via the shared
takePageWithReserve, and both catch clauses rethrow OOM as-is so it is reported
as OOM/critical failure, not corruption.
--
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]
