Fix MapperService StackOverflowError#23605
Conversation
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
|
Thanks for the pull request @jkiang13; can you please sign the CLA and add a test case that would fail on current master and is addressed by your patch? |
|
I did sign the CLA (perhaps I did it out of order of submitting this pull request). I am actually not sure how to write a test case for this, as generating a StackOverflowError would kill the JVM running the test. If you look at the referenced issue I have a simple script that reproduces the issue. |
I think that you think this because of the uncaught exception handler. It does not apply to tests, you can write a test that produces a stack overflow without killing the JVM (I did it in a recent test even). With this information, do you want to give it a try?
I see it, and I appreciate, and it makes me think that you can write a test here. A script will not suffice though, it needs to be something we can run with every CI run. |
There was a problem hiding this comment.
The stored structures don't need to be put into a fresh set on every call. The proper solution here is to only wrap the structures by unmodifiable* if they changed, i.e.
if (parentTypes != this.parentTypes) {
parentTypes = Collections.unmodifiableSet(parentTypes);
}
|
Also, the test here can simply be a unit test that does a mapping update that does not change parentTypes. The value returned by |
|
bonus points if you do the same for |
9da6d01 to
41ea6ef
Compare
41ea6ef to
f54351d
Compare
… the first merge with DocumentMappers
|
My thought was that copying the Set always up front seemed worth it for the simplicity, especially when it looks like subtlety around these collections probably contributed to this bug arising in the first place. But I will of course defer to you folks. Take a look at the subsequent commits. |
ywelsch
left a comment
There was a problem hiding this comment.
LGTM, thanks for the contribution.
MapperService#parentTypes is rewrapped in an UnmodifiableSet in MapperService#internalMerge every time the cluster state is updated. After thousands of updates the collection is wrapped so deeply that calling a method on it generates a StackOverflowError. Closes #23604
MapperService#parentTypes is rewrapped in an UnmodifiableSet in MapperService#internalMerge every time the cluster state is updated. After thousands of updates the collection is wrapped so deeply that calling a method on it generates a StackOverflowError. Closes #23604
MapperService#parentTypes is rewrapped in an UnmodifiableSet in MapperService#internalMerge every time the cluster state is updated. After thousands of updates the collection is wrapped so deeply that calling a method on it generates a StackOverflowError. Closes #23604
* master: Fix typo in allocation explain API docs Add unit tests for ReverseNestedAggregator (elastic#23651) Revert "Revert "Build: Upgrade min gradle to 3.3 (elastic#23544)"" Revert "Build: Upgrade min gradle to 3.3 (elastic#23544)" Build: Upgrade min gradle to 3.3 (elastic#23544) Fix took assertion in response filter test Search took time should use a relative clock Adds toString() to snapshot operations in progress Docs: fix a typo in transport client's put-mapping.asciidoc (elastic#23607) Use include-tagged macro for high level client docs (elastic#23438) Update fill-column in .dir-locals.el to 100 characters Setup keystore during integration tests (elastic#22966) Fix typo 'Elastisearch' -> 'Elasticsearch' (elastic#23633) Comment and blank line cleanups (elastic#23647) docs: guidelines for students and teachers (elastic#23648) Fix MapperService StackOverflowError (elastic#23605)
MapperService#parentTypes is rewrapped in an UnmodifiableSet in MapperService#internalMerge every time the cluster state is updated. After thousands of updates the collection is wrapped so deeply that calling a method on it generates a StackOverflowError.
I have been running this patch in my cluster to address issue #23604