Operating System: All
Platform: All

Typically, a LCV is due to a class being loaded by two different classloaders, and a reference to one of them is sent in some way to a class that was loaded by the other classloader, resulting in trying to resolve the same class in two different classloaders, which will result in the LCV.

You seem to be saying instead that the likely root cause is two different versions of the same class being referenced, even by the same classloader. Would that even cause a LCV?

David, I've managed to fix this one; will you merge https://git.opendaylight.org/gerrit/#/c/38169/ ASAP? (It's blocking me for the roll-out of https://github.com/vorburger/opendaylight-eclipse-setup.)

The problem was indeed an ANTLR (v4) classpath conflict between the Checkstyle Eclipse Plugin, which "exposes" (leaks) ANTLR packages, and yangide which pulled the wrong version of ANTLR (from Checkstyle) when it was doing Import-Package: org.antlr.v4.runtime in com.cisco.yangide.core. That of course was meant to get the one as Export-Package: from com.cisco.yangide.yangparser - but that's not how OSGi works for an Import-Package - which is exactly why I avoid them if at all possible!

The easiest solution to me seemed to completely kill the com.cisco.yangide.yangparser bundle (which wasn't a "yangparser" at all anyway; if anything this should have rather been called com.cisco.yangide.yangtools.libs? the parser wrapper code is inside a package in yangide.core), and pull ANTLR & Co. lib/*.jar into yangide.core, as that is the only plugin requiring those, and only internally, so no need to Export-Package ANTLR (thus avoiding any overlap with the Checkstyle one).

Personally I would argue that what the Checkstyle Eclipse Plugin does isn't entirely kosher either (I don't see why it needs to expose ANTLR externally; it just needs itself, internally), but as what yangparser was doing was not optimal either, and easier for us to fix ourselves, I choose to go that route. I've raised https://sourceforge.net/p/eclipse-cs/bugs/403/ with eclipse-cs just in case they want to do a clean up on their side as well (but we DO NOT / SHOULD NOT / CANNOT wait for that here; again what yandide was doing wasn't right anyway).

The problem with removing the yangparser plugin and just putting those jars into core is that there are other plugins that require one or more of those jars. You're correct it should have had a more generic name, but the list of required jars started as only the yang parser jars, and then I discovered others that were required by other plugins.

Is there a way to specify a variation of "Import-Package" from the importing plugins so that they get the class from the "yangparser" bundle, instead of some effectively random bundle (checkstyle, in this instance)? Again, I certainly agree that the name of the bundle should be more generic, but at this point I'd like to focus on a "smaller" change.

> The problem with removing the yangparser plugin and just putting those jars into core is that there are other plugins that require one or more of those jars.

I'm not entirely sure this is correct / actually a real problem - could you point to a specific issue you face, like mention another non-core plugin which you believe with this change does not have access to these artifacts obtained from Maven anymore, or a new bug/functional problem I can investigate that this change would introduce? I've cleaned up all the plugins that depend on yangparser and tested yangide, to the best of my knowledge, and it appeared to still work (and not have this LinkageError anymore).. although I'm not very familiar with its functionality, so I may have missed something; would you be willing to test it as well and shout if this proposed change breaks anything instead of only fixing the LinkageError? If it fixed the LinkageError, will you merge it?

> Is there a way to specify a variation of "Import-Package" from the importing plugins so that they get the class from the "yangparser" bundle

Nope.

(In reply to Michael Vorburger from comment #2)
> David, I've managed to fix this one; will you merge
> https://git.opendaylight.org/gerrit/#/c/38169/ ASAP? (It's blocking me for
> the roll-out of https://github.com/vorburger/opendaylight-eclipse-setup.)
>
> The problem was indeed an ANTLR (v4) classpath conflict between the
> Checkstyle Eclipse Plugin, which "exposes" (leaks) ANTLR packages, and
> yangide which pulled the wrong version of ANTLR (from Checkstyle) when it
> was doing Import-Package: org.antlr.v4.runtime in com.cisco.yangide.core.
> That of course was meant to get the one as Export-Package: from
> com.cisco.yangide.yangparser - but that's not how OSGi works for an
> Import-Package - which is exactly why I avoid them if at all possible!
>
> The easiest solution to me seemed to completely kill the
> com.cisco.yangide.yangparser bundle (which wasn't a "yangparser" at all
> anyway; if anything this should have rather been called
> com.cisco.yangide.yangtools.libs? the parser wrapper code is inside a
> package in yangide.core), and pull ANTLR & Co. lib/*.jar into yangide.core,
> as that is the only plugin requiring those, and only internally, so no need
> to Export-Package ANTLR (thus avoiding any overlap with the Checkstyle one).

Can we get back to understanding what would be the minimum required change to fix this problem?

If I understand this correctly, the "root cause" is the last statement I quoted here. When we define a plugin that does "export-package" of the antlr package, you seem to be saying that that does something very non-obvious, but that doesn't seem right. Isn't the real problem that I have to do "import-package" in the plugin that depends on this plugin? If I instead used "maven-dependency-plugin" in the plugin that actually requires the jar, along with the "Build-Classpath" entry, then I don't end up needing an "import-package" statement, which avoids the problem of pulling in the class from the checkstyle jar?

So, if I'm understanding this correctly, I need to determine each plugin that actually requires the antlr jar, and use "m-d-p" directly in that plugin, and remove the reference from the "yangparser" plugin. If I actually find two plugins that need the antlr jar, I just have to do the same thing in both, even though I'll be duplicating the jar file. I don't know for certain that there is more than one plugin that requires the jar. I thought there was, but I will step through and determine this.

I believe this is the "minimum" change required. After I determine this works, I can do more cleanup, but I want to avoid making multiple changes at once.

> real problem that I have to do "import-package" in the plugin that depends on this plugin

Yes, the real problem is the "import-package" not the "Export-Package", agreed.

> two plugins that need the antlr jar, I just have to do the same thing in both, even though I'll be duplicating the jar file

No, please do not do this; not only is this "ugly", more importantly it's wrong, and may lead to hard to track class loader issues, because you'll then get ANTLR from the duplicate JAR loaded twice, and if the plugins have dependencies, it could be a mess.

I don't really understand your reluctance, it's not such a huge change; I think the risk is minimal - but your call, fair enough! (If I can do anything to alleviate your "fear" of this change, please let me know.) But one new thought I did have for an alternative, if you really want to spend more time on this instead of just merging my proposed change, is the following which I realized in retrospect too late to be motivated to redo the work of my proposed change (which works, solves this bug, and simplifies the code, but anyway):

It's possible that simply making the yangide.core Require-Bundle: yangide.yangparser would also solve this problem ... worth a try, if you have the motivation and time (I do not). The idea being that, given that the root cause of the problem is the "import-package" of (an, any..) ANTLR, if you specify WHICH bundle you depend on (which offers ANTLR, implictly through it's export), this MAY work as alternative solution.

Best of luck!

(In reply to Michael Vorburger from comment #6)
> > real problem that I have to do "import-package" in the plugin that depends on this plugin
>
> Yes, the real problem is the "import-package" not the "Export-Package",
> agreed.
>
> > two plugins that need the antlr jar, I just have to do the same thing in both, even though I'll be duplicating the jar file
>
> No, please do not do this; not only is this "ugly", more importantly it's
> wrong, and may lead to hard to track class loader issues, because you'll
> then get ANTLR from the duplicate JAR loaded twice, and if the plugins have
> dependencies, it could be a mess.

If there are two plugins that both need the antlr jar, how would you propose I do it then? I'm well aware of the fact that it's "ugly". If we're truly in a position where we absolutely cannot do an "import-package" of the antlr classes from anywhere, then we have to ensure that the antlr jar is "bundled" into each plugin. In that case, "ugly" is our only choice.

> I don't really understand your reluctance, it's not such a huge change; I
> think the risk is minimal - but your call, fair enough! (If I can do
> anything to alleviate your "fear" of this change, please let me know.) But
> one new thought I did have for an alternative, if you really want to spend
> more time on this instead of just merging my proposed change, is the
> following which I realized in retrospect too late to be motivated to redo
> the work of my proposed change (which works, solves this bug, and simplifies
> the code, but anyway):
>
> It's possible that simply making the yangide.core Require-Bundle:
> yangide.yangparser would also solve this problem ... worth a try, if you
> have the motivation and time (I do not). The idea being that, given that
> the root cause of the problem is the "import-package" of (an, any..) ANTLR,
> if you specify WHICH bundle you depend on (which offers ANTLR, implictly
> through it's export), this MAY work as alternative solution.

I'm already doing that. Are you suggesting that if I have the "require-bundle" in place, I don't need the "import-package"? That seems unlikely to me. From what we're seeing, the OSGi container doesn't have any intelligence to "link" the fact that we should only pull packages referred to in "import-package" from bundles that we have specified with "require-bundle".
>
> Best of luck!

> If there are two plugins that both need the antlr jar, how would you propose I do it then?

You would have to put the ANTLR JAR in one plugin (e.g. yangide.yangparser) on which the two other plugins (e.g. yangide.core + ...) depend, via Require-Bundle.

> If we're truly in a position where we absolutely cannot do an "import-package" of the antlr classes from anywhere

Yes we are.

> then we have to ensure that the antlr jar is "bundled" into each plugin.

Nope.

> In that case, "ugly" is our only choice.

No, because it's not just "ugly" but also "wrong".

> Are you suggesting that if I have the "require-bundle" in place, I don't need the "import-package"?

Yes.

> That seems unlikely to me.

Try it?

(In reply to Michael Vorburger from comment #8)
> > If there are two plugins that both need the antlr jar, how would you propose I do it then?
>
> You would have to put the ANTLR JAR in one plugin (e.g. yangide.yangparser)
> on which the two other plugins (e.g. yangide.core + ...) depend, via
> Require-Bundle.
>
> > If we're truly in a position where we absolutely cannot do an "import-package" of the antlr classes from anywhere
>
> Yes we are.
>
> > then we have to ensure that the antlr jar is "bundled" into each plugin.
>
> Nope.
>
> > In that case, "ugly" is our only choice.
>
> No, because it's not just "ugly" but also "wrong".
>
> > Are you suggesting that if I have the "require-bundle" in place, I don't need the "import-package"?
>
> Yes.
>
> > That seems unlikely to me.
>
> Try it?

I'm testing this idea this morning. I already had the "Require-Bundle" in place, but I also was specifying the antlr packages in "Import-Package". I've removed those now. I haven't seen the linkage error in initial testing, but the annoying thing about this bug is that it hasn't been happening all of the time.

At this point, I'm wondering what is the POINT of the "Import-Package" directive? If it's irrelevant (and causes problems) in this situation, when is it actually needed?

I merged a change that just removed the Import-Package reference, but left the jars in yangparser. I imagine this is still vulnerable to the linkage error, but I haven't seen it in a while. If it comes up again, I guess we'll have to move the antlr jar from yangparser into core (using maven-dependency-plugin).

> I'm wondering what is the POINT of the "Import-Package" directive? If it's irrelevant (and causes problems) in this situation, when is it actually needed?

My understanding is that there is some history here: OSGi purists will tell you that Import-Package is the (only) way to go. It is actually possible to version Java Package exports and imports. Eclipse forced OSGi to introduce "Require-Bundle" (because that's how the Eclipse plugins worked before they switched to OSGi).

> I imagine this is still vulnerable to the linkage error, but I haven't seen it in a while.

I can confirm that I'm not seeing the original problem above anymore either.

I've opened new YANGIDE-10 for a different error I'm seeing now (which may or may not be an impact of the removed the Import-Package reference; let's track that under YANGIDE-10).

I suggest that we close this bug.

Considering how elusive this error has been, I'm a little hesitant to close this yet, considering I'm not sure what we've done to fix it. I find it hard to believe just removing the "Import-Package" reference (leaving the "Require-Bundle" reference) has done it.

We've made a fix that appears to have fixed it, but we never completely understood the problem, so I wouldn't be completely surprised if it showed up again, but we can call it fixed for now.