Closed reta closed 2 years ago
@alzimmermsft @rickle-msft guys, if it makes sense to you, I would be happy to submit the pull request to enhance page iterables (ContinuablePagedByXxx) to treat empty and null token as equivalent
@reta Thank you for opening this issue and for the thorough description and suggestions. I think @alzimmermsft will be most equipped to respond to this when he gets back from vacation in a few days
Thank you for reporting this @reta. I'm taking a look into the root issue, I'll have an update soon.
@reta I've completed my preliminary troubleshooting of this issue, and this was an amazing find on your part!
What you've found is a difference in paging termination between PagedFlux
and PagedIterable
(more specifically in their super classes but these are what is exposed in the Storage SDKs). PagedIterable
has a divergent code path from PagedFlux
due to the way that Reactor had, and possibly still has, handled transitioning a reactive stream into an Iterable
or Stream
where the next element retrieval would eagerly populate the next-next element resulting in errant page requests.
A few months ago there was logic added into the PagedFlux
class hierarchy that allowed for a Predicate
to be passed to determine when paging should terminate and the default changed from continuation token == null
to continuation token == "" || continuation token == null
when a String
based continuation token was being used (continuation token == null
is still the default for non-String
continuation tokens). Unfortunately, there was an oversight on PagedIterable
and PagedFlux
using divergent code paths which resulted in PagedIterable
not using the paging termination Predicate
. I've filed PR #26139 to resolve this difference.
One quick ask I have from your side is using the same setup could you try using the async paging to double verify my statements above. It should be as simple as:
BlobContainerAsyncClient asyncContainerClient = null; // builder logic here
List<BlobItem> blobItems = asyncContainerClient.listBlobs().listBlobs().collectList().block();
If the above was true this should terminate and not run infinitely.
Thanks a lot for looking into it @alzimmermsft , I could try async paging, but would it actually page? In the snippet you have provided it looks like all blobs are going to be collected all at once.
Thanks a lot for looking into it @alzimmermsft , I could try async paging, but would it actually page? In the snippet you have provided it looks like all blobs are going to be collected all at once.
Yeah, that is correct that all blobs are going to be collected at once but underneath it is just consuming paged responses until paging terminates. A small change to be closer to what you've posted in the original issue statement would be:
asyncContainerClient.listBlobs(listBlobsOptions).timeout(timeout())
.map(blobItem -> ....)
.block();
The timeout will cause the reactive stream to throw an error if a page isn't received before the duration completes and is reset each time a page is received. Block will make it so the application won't continue running while paging is going on.
Describe the bug It turns out that Azure SDK v12 is very sensitive to the
XMLInputReader
implementation (coming fromJacksonAdapter
) and heavily relies on the fact that empty XML elements / attributes are going to be nullified.However, sadly, it highly depends on
XMLInputReader
instance being picked up at runtime: theWoodstox
does that, whereas the default one from JDKcom.sun.org.apache.xerces.internal.impl.XMLStreamReaderImpl
does not. It leads to infinite loop withinBlobContainerClient::listBlobsByHierarchy
andBlobContainerClient::listBlobs
- the page iterables (ContinuablePagedByXxx
) only understandsnull
as termination condition.The
XMLInputReader
instance is created by Jackson'sXmlFactory
and is used byFromXmlParser
to parse XML payloads.Exception or Stack Trace There is no stack trace, the
BlobContainerClient::listBlobsByHierarchy
andBlobContainerClient::listBlobs
never return trying to fetch the next pages by empty continuation token.To Reproduce It is very easy to reproduce, here is the code snippet with
ListBlobsFlatSegmentResponse
response example:It uses JDK17 syntax but reproducible on any modern JDKs. When run with
-Djavax.xml.stream.XMLInputFactory=com.sun.xml.internal.stream.XMLInputFactoryImpl
, the output of the program is:When run without
-Djavax.xml.stream.XMLInputFactory
(or equivalent of-Djavax.xml.stream.XMLInputFactory=com.ctc.wstx.stax.WstxInputFactory
), the output of the program is:Code Snippet
If timeout is not specified, the
listBlobs
never returns.Expected behavior The function should return normally.
Screenshots If applicable, add screenshots to help explain your problem.
Setup (please complete the following information):
Additional context This particular issue is only happening when non-Woodstox
XMLInputReader
is picked up, there are multiple options to this particular problem: a) Enhance page iterables (ContinuablePagedByXxx
) to treat empty andnull
token as equivalent b) Allow to provide ownXmlFactory
instance inJacksonAdapter
throughXmlMapper.builder(XmlFactory)
constructor (which covers bothXMLInputReader
andXMLInputWriter
) c) UseWoodstox
explicitly in theJacksonAdapter
while configuringXmlMapper
I believe the option a) is the most appropriate thing to do.
Information Checklist Kindly make sure that you have added all the following information above and checkoff the required fields otherwise we will treat the issuer as an incomplete report