Fix MapperService StackOverflowError

[jkiang13]

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

[elasticmachine]

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?

[jasontedor]

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?

[jkiang13]

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.

[jasontedor]

I am actually not sure how to write a test case for this, as generating a StackOverflowError would kill the JVM running the test.

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?

kOverflowError would kill the JVM running the test. If you look at the referenced issue I have a simple script that reproduces the issue.

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.

[ywelsch]

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);
}

[ywelsch]

Also, the test here can simply be a unit test that does a mapping update that does not change parentTypes. The value returned by getParentTypes() should then be the same map as we had before the mapping update (reference identity).

[ywelsch]

bonus points if you do the same for fullPathObjectMappers.

[jkiang13]

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]

LGTM, thanks for the contribution.