Uploaded image for project: 'logback'
  1. logback
  2. LOGBACK-159

JaninoEventEvaluator causes Janino to use the TCCL

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Won't Fix
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      This is a regression compared to 0.9.24 which correctly specified logback's class loader as Janino's "parent" class loader.

      Details: http://mailman.qos.ch/pipermail/logback-user/2012-January/002917.html

        Activity

        Hide
        glyn Glyn Normington added a comment -

        I have provided a fix and a test in [1] and issued a pull request to [2]. I have verified the fix works under Virgo ([3]).

        The fix is along the lines recommended by Arno Unkrig in the referenced thread, but I am a little concerned that Janino is not thread safe when used in this way. I have raised this concern on the janino-user mailing list ([4]). However, this is a Janino issue. The only relevance is that the proposed fix to this issue potentially exposes logback to the thread safety issue. So it may be worth deferring pulling this fix in until we are sure Janino is suitably thread safe.

        As for the test, it could be made more compact with the use of a suitable mocking library, but I wasn't sure which one to choose, so I did without. Also, it avoids creating a custom class loader to test that Janino's "parent" class loader has been correctly set. It does this by reflectively accessing a private field from Janino. Clearly this test is likely to break if Janino changes that field, but at least the current test is small and easily understood.

        [1] https://github.com/glyn/logback/commit/deee1536d3ea65be3dfe0f9123d3b87af8b6e31c
        [2] https://github.com/ceki/logback
        [3] https://bugs.eclipse.org/bugs/show_bug.cgi?id=333920#c16
        [4] http://old.nabble.com/Re%3A-Janino-and-OSGi--p33128536.html

        Show
        glyn Glyn Normington added a comment - I have provided a fix and a test in [1] and issued a pull request to [2] . I have verified the fix works under Virgo ( [3] ). The fix is along the lines recommended by Arno Unkrig in the referenced thread, but I am a little concerned that Janino is not thread safe when used in this way. I have raised this concern on the janino-user mailing list ( [4] ). However, this is a Janino issue. The only relevance is that the proposed fix to this issue potentially exposes logback to the thread safety issue. So it may be worth deferring pulling this fix in until we are sure Janino is suitably thread safe. As for the test, it could be made more compact with the use of a suitable mocking library, but I wasn't sure which one to choose, so I did without. Also, it avoids creating a custom class loader to test that Janino's "parent" class loader has been correctly set. It does this by reflectively accessing a private field from Janino. Clearly this test is likely to break if Janino changes that field, but at least the current test is small and easily understood. [1] https://github.com/glyn/logback/commit/deee1536d3ea65be3dfe0f9123d3b87af8b6e31c [2] https://github.com/ceki/logback [3] https://bugs.eclipse.org/bugs/show_bug.cgi?id=333920#c16 [4] http://old.nabble.com/Re%3A-Janino-and-OSGi--p33128536.html
        Hide
        tony19 Tony Trinh added a comment -

        Hi Glyn,

        The only relevance is that the proposed fix to this issue potentially exposes logback to the thread safety issue. So it may be worth deferring pulling this fix in until we are sure Janino is suitably thread safe.

        Have you been able to verify the thread safety?

        As for the test, it could be made more compact with the use of a suitable mocking library, but I wasn't sure which one to choose, so I did without.

        The mocking library we use is mockito.

        Thanks.

        Show
        tony19 Tony Trinh added a comment - Hi Glyn, The only relevance is that the proposed fix to this issue potentially exposes logback to the thread safety issue. So it may be worth deferring pulling this fix in until we are sure Janino is suitably thread safe. Have you been able to verify the thread safety? As for the test, it could be made more compact with the use of a suitable mocking library, but I wasn't sure which one to choose, so I did without. The mocking library we use is mockito. Thanks.
        Hide
        glyn Glyn Normington added a comment -

        Hi Tony

        I have not verified Janino is now thread safe.

        Thanks for the info. re. mockito, but I won't be able to update the pull request to use it as I am now busy on other projects.

        Feel free to close this issue if you need more involvement from me as I won't be able to provide it.

        Regards,
        Glyn

        Show
        glyn Glyn Normington added a comment - Hi Tony I have not verified Janino is now thread safe. Thanks for the info. re. mockito, but I won't be able to update the pull request to use it as I am now busy on other projects. Feel free to close this issue if you need more involvement from me as I won't be able to provide it. Regards, Glyn
        Hide
        tony19 Tony Trinh added a comment -

        Ok, thanks for the update. The issue can be reopened (or a new one created) if someone needs this change.

        Show
        tony19 Tony Trinh added a comment - Ok, thanks for the update. The issue can be reopened (or a new one created) if someone needs this change.

          People

          • Assignee:
            tony19 Tony Trinh
            Reporter:
            glyn Glyn Normington
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: