Skip to content
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.

Sign up for GitHub

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

Jump to bottom

NPE in PagedBytesReference #5667

Closed
mattweber opened this issue Apr 2, 2014 · 7 comments · Fixed by #5677
Closed

NPE in PagedBytesReference #5667

mattweber opened this issue Apr 2, 2014 · 7 comments · Fixed by #5677
Assignees
@s1monw
Labels
>bug v1.2.0 v2.0.0-beta1

Comments

@mattweber
Copy link
Contributor

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

 if ("filter".equals(currentFieldName)) {
    lookupFilter = XContentFactory.contentBuilder(parser.contentType());
    lookupFilter.copyCurrentStructure(parser);
}

I can push up the rebased PR that is failing if you need it to test. Let me know.

/cc @s1monw @hhoffstaette
@s1monw
Copy link
Contributor

s1monw commented Apr 2, 2014

a test for this would be awesome!

@s1monw s1monw self-assigned this Apr 2, 2014
@mattweber
Copy link
Contributor Author

Let me see if I can get something specific. #3278 is pretty complex.

@mattweber
Copy link
Contributor Author

Ironically, trying to write a test for this triggered another.

Drop this into XContentBuilderTests.java

    @Test
    public void testCopyCurrentStructure() throws Exception {
        XContentBuilder builder = XContentFactory.contentBuilder(XContentType.JSON);
        builder.startObject()
            .field("test", "test field")
            .startObject("filter")
                .startObject("terms");

        // up to 20k random terms
        int numTerms = randomInt(20000) + 1;
        List<String> terms = new ArrayList<>(numTerms);
        for (int i = 0; i < numTerms; i++) {
            terms.add("test" + i);
        }

        builder.field("fakefield", terms).endObject().endObject().endObject();

        XContentParser parser = XContentFactory.xContent(XContentType.JSON).createParser(builder.bytes());

        XContentBuilder filterBuilder = null;
        XContentParser.Token token;
        String currentFieldName = null;
        assertThat(parser.nextToken(), equalTo(XContentParser.Token.START_OBJECT));
        while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
            if (token == XContentParser.Token.FIELD_NAME) {
                currentFieldName = parser.currentName();
            } else if (token.isValue()) {
                if ("test".equals(currentFieldName)) {
                    assertThat(parser.text(), equalTo("test field"));
                }
            } else if (token == XContentParser.Token.START_OBJECT) {
                if ("filter".equals(currentFieldName)) {
                    filterBuilder = XContentFactory.contentBuilder(parser.contentType());
                    filterBuilder.copyCurrentStructure(parser);
                }
            }
        }

        assertNotNull(filterBuilder);
        parser = XContentFactory.xContent(XContentType.JSON).createParser(filterBuilder.bytes());
        assertThat(parser.nextToken(), equalTo(XContentParser.Token.START_OBJECT));
        assertThat(parser.nextToken(), equalTo(XContentParser.Token.FIELD_NAME));
        assertThat(parser.currentName(), equalTo("terms"));
        assertThat(parser.nextToken(), equalTo(XContentParser.Token.START_OBJECT));
        assertThat(parser.nextToken(), equalTo(XContentParser.Token.FIELD_NAME));
        assertThat(parser.currentName(), equalTo("fakefield"));
        assertThat(parser.nextToken(), equalTo(XContentParser.Token.START_ARRAY));
        int i = 0;
        while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
            assertThat(parser.text(), equalTo(terms.get(i++)));
        }

        assertThat(i, equalTo(terms.size()));
    }

Run test with:

mvn test -Dtests.seed=F330AACFAFD76C8B -Dtests.class=org.elasticsearch.common.xcontent.builder.XContentBuilderTests -Dtests.method=testCopyCurrentStructure -Dtests.prefix=tests -Dfile.encoding=UTF-8 -Duser.timezone=America/Los_Angeles

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.

@mattweber
Copy link
Contributor Author

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.

@mattweber
Copy link
Contributor Author

Need a PR for the test or is the code above enough?

@hhoffstaette
Copy link

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.

@s1monw s1monw mentioned this issue Apr 3, 2014
Merged
@mattweber
Copy link
Contributor Author

Awesome @s1monw! I will give this a try shortly!

@s1monw s1monw closed this as completed in cd552e7 Apr 3, 2014
s1monw added a commit that referenced this issue Apr 3, 2014
@s1monw
Take stream position into account when calculating remaining length
3c9ad21 
Currently `PagedBytesReferenceStreamInput#read(byte[],int,int)` ignores
the current pos and that causes to read past EOF

Closes #5667
@s1monw s1monw added bug labels Apr 3, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@s1monw s1monw

Labels
>bug v1.2.0 v2.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Take stream position into account when calculating remaining length s1monw/elasticsearch
3 participants
@mattweber @hhoffstaette @s1monw