Fix stackoverflow with large pages in paginator #6661
Conversation
dagnir
force-pushed
the
dongie/gh-6411
branch
3 times, most recently
from
4125690 to
09a3a0c
Compare
dagnir
force-pushed
the
dongie/gh-6411
branch
2 times, most recently
from
124360b to
35c79e4
Compare
Previously, signalig onNext() to the subscriber was done via recursion, pulling elements from an iterator over the current page returned by the service. However, this can quickly lead to a stackoverflow error since the stack will grow linearly with the size of the page. - Replace sendNextElement recursion with a loop - Ensure that handleRequests does not recurse into itself fixes #6411
| public final class ItemsSubscription<ResponseT, ItemT> extends PaginationSubscription<ResponseT> { | ||
| private final Function<ResponseT, Iterator<ItemT>> getIteratorFunction; | ||
| private volatile Iterator<ItemT> singlePageItemsIterator; | ||
| private final AtomicBoolean handlingRequests = new AtomicBoolean(); |
There was a problem hiding this comment.
I assume we're effectively using this as a "try-lock" (ie, return quickly if another thread is already in this method)?
There was a problem hiding this comment.
Yep basically
edit: not just another thread, but also prevent handleRequests from reentering, which could happen for example if the subscriber does something like
void onNext(String item) {
subscription.request(1);
}
| private final Function<ResponseT, Iterator<ItemT>> getIteratorFunction; | ||
| private volatile Iterator<ItemT> singlePageItemsIterator; | ||
| private final AtomicBoolean handlingRequests = new AtomicBoolean(); | ||
| private volatile boolean awaitingNewPage = false; |
There was a problem hiding this comment.
Why are we using an AtomicBoolean for handlingRequests but a volatile for awaitingNewPage? I'm trying to work through different cases.... when handleRequests sets awaitingNewPage to true (line 78), we create a next page future and then the whenComplete on that future which may run in a different thread and so the awaitingNewPage getting set to false may only apply in that thread?
There was a problem hiding this comment.
Only reason awaitingNewPage is volatile is I didn't need to use a CAS for it
so the awaitingNewPage getting set to false may only apply in that thread?
Hmm I might not completely understand the question here, but it's volatile so reads on awaitingNewPage will never be stale. e.g. thread T1 setting awaitingNewPage = true (atomic), and T2 reading it after the write will always return true (never false).
|
2 participants
Motivation and Context
Previously, signalig onNext() to the subscriber was done via recursion, pulling elements from an iterator over the current page returned by the service. However, this can quickly lead to a stackoverflow error since the stack will grow linearly with the size of the page.
Modifications
Testing
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License