Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-7883

Remove references to Thread#getContextClassLoader() from Lucene/Solr codebase

Details

    • Improvement
    • Status: Resolved
    • Critical
    • Resolution: Fixed
    • None
    • 7.0
    • None
    • None
    • New

    Description

      As discussed in LUCENE-7873, the use of Thread.currentThread().getContextClassLoader() is in most cases a bug caused by sloppy usage of ClassLoader APIs and should be replaced by getClass().getClassLoader().

      In Lucene we only have the ClassPathResourceLoader, but this one can be solved by removing the "default" constructor. It only affects CustomAnalyzer, that should also be extended in Lucene 7.

      The uses in Solr and its test are all to be replaced by getClass().getClassLoader() (except some workaround with clustering using a try-finally). This is in most cases historical code, when Solr was a web application to workaround some buggy webapp containers. In the current state, the code is "just wrong".

      Finally, we should add java.lang.Thread#getContextClassLoader() to forbidden-apis.

      I'd like to get this into Lucene 7, as it affects some APIs in Lucene.

      Attachments

        1. LUCENE-7883.patch
          24 kB
          Uwe Schindler

        Issue Links

          Activity

            tomoko Tomoko Uchida added a comment -

            This issue was moved to GitHub issue: #8934.

            tomoko Tomoko Uchida added a comment - This issue was moved to GitHub issue: #8934 .
            uschindler Uwe Schindler added a comment -

            I added changes and migrate entries and committed to master (7.0).

            uschindler Uwe Schindler added a comment - I added changes and migrate entries and committed to master (7.0).

            Commit 5de15ff403fbf4afe68718151617e6104f7e3888 in lucene-solr's branch refs/heads/master from [~thetaphi]
            [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=5de15ff ]

            LUCENE-7883: Lucene/Solr no longer uses the context class loader when resolving resources

            jira-bot ASF subversion and git services added a comment - Commit 5de15ff403fbf4afe68718151617e6104f7e3888 in lucene-solr's branch refs/heads/master from [~thetaphi] [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=5de15ff ] LUCENE-7883 : Lucene/Solr no longer uses the context class loader when resolving resources
            uschindler Uwe Schindler added a comment -

            Unfortunately the latest test failures on master make it hard to differentiate between failures caused by my changes and ones already there. But all failures in tests that I see here, look like the ones Jenkins is drinking with his beers!

            This is not a good state, the test suite should pass for a clean checkout.

            uschindler Uwe Schindler added a comment - Unfortunately the latest test failures on master make it hard to differentiate between failures caused by my changes and ones already there. But all failures in tests that I see here, look like the ones Jenkins is drinking with his beers! This is not a good state, the test suite should pass for a clean checkout.
            uschindler Uwe Schindler added a comment -

            I did some live test with the standalone techproducts example. I have seen no issues, so I think this should be fine to commit. I will add a CHANGES entry in both Lucene and Solr, because this affects both projects.

            uschindler Uwe Schindler added a comment - I did some live test with the standalone techproducts example. I have seen no issues, so I think this should be fine to commit. I will add a CHANGES entry in both Lucene and Solr, because this affects both projects.
            dweiss Dawid Weiss added a comment -

            Looks good to me. I'll check why the context classloader is required in clustering later on. I think the case was that clustering was under shared libraries and resources loaded per-core couldn't figure where to load classes from.

            dweiss Dawid Weiss added a comment - Looks good to me. I'll check why the context classloader is required in clustering later on. I think the case was that clustering was under shared libraries and resources loaded per-core couldn't figure where to load classes from.
            uschindler Uwe Schindler added a comment -

            Patch removing context classloader usage. Tests seem to pass, unfortunately Solr trunk is very unstable. Some unrelated tests also fail on Jenkins, so I cannot be sure all is fine.

            This patch also adds context class loaders on te forbidden api list. Because of that I used the withContextClassLoader(ClassLoader, () -> { ... }) lambda method.

            uschindler Uwe Schindler added a comment - Patch removing context classloader usage. Tests seem to pass, unfortunately Solr trunk is very unstable. Some unrelated tests also fail on Jenkins, so I cannot be sure all is fine. This patch also adds context class loaders on te forbidden api list. Because of that I used the withContextClassLoader(ClassLoader, () -> { ... }) lambda method.
            dweiss Dawid Weiss added a comment -

            I can look into the clustering plugin's use of it. I recall it was unfortunately required, but will have to go into this again to remind myself why.

            dweiss Dawid Weiss added a comment - I can look into the clustering plugin's use of it. I recall it was unfortunately required, but will have to go into this again to remind myself why.

            People

              uschindler Uwe Schindler
              uschindler Uwe Schindler
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Slack