SOLR-16801: Set previous context class loader after loading plugin #1645

thomaswoeckinger · 2023-05-16T11:57:38Z

https://issues.apache.org/jira/browse/SOLR-16801

Description

Due to SOLR-16240 context class loader was set before loading CoreContainer.

As the context class loader is not unset when loading is finished it is currently impossible to use
EmbeddedSolrServer within Tomcat and JSF, as it the whole class loading during servlet init process.

Solution

Set context class loader to original context class loader after plugin loading.

Tests

Plugin loading tests are already provided.

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide

thomaswoeckinger · 2023-05-16T12:44:32Z

@HoustonPutman plz review

HoustonPutman

So there might be plugins that aren't authentication that might need the Solr classloader on initialization. Is there are issue doing this try {} finally {} in load() instead of in initializeAuthentication()?

We could also just do this try {} finally {} logic in EmbeddedSolrServer. load(CoreContainer cc). That way it works for embedded use cases. I'm not sure if theres any gain to solving this outside of the embedded use case.

thomaswoeckinger · 2023-05-16T14:39:04Z

So there might be plugins that aren't authentication that might need the Solr classloader on initialization. Is there are issue doing this try {} finally {} in load() instead of in initializeAuthentication()?

No issue, it simply has to be decided if the usage of a custom context class loader should be as isolated as possible or not.
Both ways will work.

We could also just do this try {} finally {} logic in EmbeddedSolrServer. load(CoreContainer cc). That way it works for embedded use cases. I'm not sure if theres any gain to solving this outside of the embedded use case.

Sure, but i think it would be better to have a defined scope where this context class loader should/must be active.

At the end someone has to decide it, personally i would only use a different context class loader for the parts where it is required.

HoustonPutman · 2023-05-16T15:13:04Z

Ok I moved the logic to surround the entire load method.

For future PRs, please make them against main, not any other branch. I'm going to edit this PR to make it work though.

HoustonPutman · 2023-05-16T15:16:42Z

Do you mind if I force push to your branch_9x, I'm going to push the current main with these two commits cherry-picked on top? You will need to reset (delete and recreate) the branch after this PR gets merged, but at least we get to keep the PR history...

sonatype-lift · 2023-05-16T16:05:32Z

solr/core/src/java/org/apache/solr/core/CoreContainer.java

+      // Set the thread's contextClassLoader for any plugins that are loaded via Modules or Packages
+      // and have dependencies that use the thread's contextClassLoader
+      Thread.currentThread().setContextClassLoader(loader.getClassLoader());
+      loadInternal();


THREAD_SAFETY_VIOLATION: Unprotected write. Non-private method CoreContainer.load() indirectly writes to field noggit.JSONParser.devNull.buf outside of synchronization.
Reporting because another access to the same memory occurs on a background thread, although this access may not.

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

thomaswoeckinger · 2023-05-16T17:16:21Z

Do you mind if I force push to your branch_9x, I'm going to push the current main with these two commits cherry-picked on top? You will need to reset (delete and recreate) the branch after this PR gets merged, but at least we get to keep the PR history...

No problem, just force push

thomaswoeckinger · 2023-05-16T17:16:52Z

Ok I moved the logic to surround the entire load method.

For future PRs, please make them against main, not any other branch. I'm going to edit this PR to make it work though.

👌

HoustonPutman · 2023-05-17T18:05:04Z

Ok, it looks like I can't force push to your repo. Could you force push main:branch_9x? I think I can cherry pick the commits after that.

thomaswoeckinger · 2023-05-18T20:52:08Z

Ok, it looks like I can't force push to your repo. Could you force push main:branch_9x? I think I can cherry pick the commits after that.

Done, i pushed branch_9x over main, they are equal now

HoustonPutman · 2023-05-18T20:54:08Z

Sorry, its the other way around, you need to push main on top of your branch_9x.

thomaswoeckinger · 2023-05-18T20:56:09Z

Sorry, its the other way around, you need to push main on top of your branch_9x.

Ok, should i cherry pick the changes already, from the local branch_9x?

thomaswoeckinger · 2023-05-18T21:04:51Z

Cherry pick is already done

HoustonPutman · 2023-05-18T21:08:02Z

Tada! Thanks for the help there.

I think this is good to go. Will ask for another review and add a changelog entry.

thomaswoeckinger · 2023-05-18T21:10:29Z

Tada! Thanks for the help there.

I think this is good to go. Will ask for another review and add a changelog entry.

Okay, thx for your fast response, if any further actions are required just let me know.

This reverts commit a129a82.

…oreContainer (#1645) Co-authored-by: Thomas Wöckinger <two@silbergrau.com> Co-authored-by: Houston Putman <houston@apache.org> (cherry picked from commit 0a6a114)

thomaswoeckinger force-pushed the branch_9x branch from 8cc50ef to a1dadef Compare May 16, 2023 11:59

thomaswoeckinger changed the title ~~SOLR-164801: Set previous context class loader after loading plugin~~ SOLR-16801: Set previous context class loader after loading plugin May 16, 2023

HoustonPutman reviewed May 16, 2023

View reviewed changes

sonatype-lift bot reviewed May 16, 2023

View reviewed changes

Thomas Wöckinger and others added 2 commits May 18, 2023 22:58

SOLR-16801: Set previous context class loader after loading plugin

702a58f

Have logic still surround all of load

7ed55b9

thomaswoeckinger force-pushed the branch_9x branch from 0fab9bb to 7ed55b9 Compare May 18, 2023 21:04

HoustonPutman changed the base branch from branch_9x to main May 18, 2023 21:07

HoustonPutman requested a review from risdenk May 18, 2023 21:08

risdenk approved these changes May 18, 2023

View reviewed changes

HoustonPutman and others added 3 commits May 18, 2023 17:16

Update CHANGES.txt

a129a82

Revert "Update CHANGES.txt"

96a2f90

This reverts commit a129a82.

Try to add a changes entry again

1c334bf

HoustonPutman merged commit 0a6a114 into apache:main May 18, 2023
2 of 3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-16801: Set previous context class loader after loading plugin #1645

SOLR-16801: Set previous context class loader after loading plugin #1645

thomaswoeckinger commented May 16, 2023 •

edited

thomaswoeckinger commented May 16, 2023

HoustonPutman left a comment

thomaswoeckinger commented May 16, 2023

HoustonPutman commented May 16, 2023

HoustonPutman commented May 16, 2023

sonatype-lift bot May 16, 2023

thomaswoeckinger commented May 16, 2023

thomaswoeckinger commented May 16, 2023

HoustonPutman commented May 17, 2023

thomaswoeckinger commented May 18, 2023 •

edited

HoustonPutman commented May 18, 2023

thomaswoeckinger commented May 18, 2023 •

edited

thomaswoeckinger commented May 18, 2023

HoustonPutman commented May 18, 2023

thomaswoeckinger commented May 18, 2023

Command	Usage
`@sonatype-lift ignore`	Leave out the above finding from this PR
`@sonatype-lift ignoreall`	Leave out all the existing findings from this PR
`@sonatype-lift exclude <file\|issue\|path\|tool>`	Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file

SOLR-16801: Set previous context class loader after loading plugin #1645

SOLR-16801: Set previous context class loader after loading plugin #1645

Conversation

thomaswoeckinger commented May 16, 2023 • edited

Description

Solution

Tests

Checklist

thomaswoeckinger commented May 16, 2023

HoustonPutman left a comment

Choose a reason for hiding this comment

thomaswoeckinger commented May 16, 2023

HoustonPutman commented May 16, 2023

HoustonPutman commented May 16, 2023

sonatype-lift bot May 16, 2023

Choose a reason for hiding this comment

thomaswoeckinger commented May 16, 2023

thomaswoeckinger commented May 16, 2023

HoustonPutman commented May 17, 2023

thomaswoeckinger commented May 18, 2023 • edited

HoustonPutman commented May 18, 2023

thomaswoeckinger commented May 18, 2023 • edited

thomaswoeckinger commented May 18, 2023

HoustonPutman commented May 18, 2023

thomaswoeckinger commented May 18, 2023

thomaswoeckinger commented May 16, 2023 •

edited

thomaswoeckinger commented May 18, 2023 •

edited

thomaswoeckinger commented May 18, 2023 •

edited