Close query cache on index service creation failure

[DaveCTurner]

Today it is possible that we create the QueryCache and then fail to create
the owning IndexService and this means we do not close the QueryCache
again. This commit addresses that leak.

Fixes #48186

[elasticmachine]

Pinging @elastic/es-distributed (:Distributed/CRUD)

[original-brownbear]

LGTM :)

[jimczi]

It looks like we can have another leak here ? The index analyzers are created by the IndexService so the build could move to the IndexModule ? These constructors are also not supposed to throw exceptions so it should also be possible to delay the IllegalArgumentException to MapperService#merge ? The exception should be removed in master since it is not applicable anymore so we probably don't need to spend too much time on it but I wonder if the model is safe here since we cannot protect all constructors with closeable object against random runtime exceptions ?

[DaveCTurner]

Thanks @jimczi, see ccdc259. I'm wondering about moving the creation of MapperService/IndexFieldDataService/BitsetFilterCache/IndexCache out as well, even though I don't think these leak meaningfully. Makes this change quite a bit larger and adds a bunch more constructor parameters. WDYT?

[jimczi]

LGTM

I'm wondering about moving the creation of MapperService/IndexFieldDataService/BitsetFilterCache/IndexCache out as well, even though I don't think these leak meaningfully. Makes this change quite a bit larger and adds a bunch more constructor parameters. WDYT?

Agreed, maybe add a comment explaining why we expect an exception here and we can clean up when we remove the IllegalArgumentException in MapperService on master ?

[astefan]

LGTM

[DaveCTurner]

@jimczi Are you sure this exception handling stops being necessary when the obvious IllegalArgumentException goes away? If so, it would be better just to handle that one case. But there's a lot of code here, some of it pluggable, so I would lean towards being defensive and keeping it.

[jimczi]

Are you sure this exception handling stops being necessary when the obvious IllegalArgumentException goes away?

I am not sure, maybe this is the right protection but I wonder if throwing a runtime exception in a constructor is a bug or a feature and if all constructors that handle closeable objects should take care of these exceptions that are not declared. I am maybe overthinking this but we had the case in another constructor recently so I am fine keeping the protection if this is something that we do everywhere ?