New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NPE in PagedBytesReference #5667
Comments
a test for this would be awesome! |
Let me see if I can get something specific. #3278 is pretty complex. |
Ironically, trying to write a test for this triggered another. Drop this into XContentBuilderTests.java
Run test with:
I am wondering if my original issue is something to do with threading. Multiple shards on the same node all parsing the terms filter and calling copyCurrentStructure. Not sure, still trying to reproduce in an actual test. |
Actually this test is for the error I reported, tests run with -ea enabled which triggers the assertion. In my benchmark I didn't have -ea enabled so it hits the NPE. Running my benchmark with -ea triggers this same assertion. |
Need a PR for the test or is the code above enough? |
It is not clear to me which assertion you are talking about in #5667 (comment) - are you referring to the assert right before the arraycopy or one in your tests? Regardless, it's not surprising that this would fail when the same PBR and/or stream is used & mutated by multiple threads. Hitting the NPE has btw nothing to do with the assertion, and would just be a side effect of corruption: the condition for the assert still implies that ref.bytes is not null and valid. |
Awesome @s1monw! I will give this a try shortly! |
Currently `PagedBytesReferenceStreamInput#read(byte[],int,int)` ignores the current pos and that causes to read past EOF Closes #5667
I rebased #3278 to latest master and one of my benchmarks is throwing a NPE. Traced it down to ref.bytes being null for the array copy. See PagedBytesReference.java#L448.
Not sure why, but this is the chunk of code that triggers this in my PR is:
TermsFilterParser.java#L135
I can push up the rebased PR that is failing if you need it to test. Let me know.
/cc @s1monw @hhoffstaette
The text was updated successfully, but these errors were encountered: