Opened 3 years ago

Closed 3 years ago

#1679 closed defect (fixed)

remove trouble causing commons-logging.jar from i2p install (blocks #1678)

Reported by: kay Owned by:
Priority: major Milestone: 0.9.24
Component: apps/jetty Version: 0.9.22
Keywords: i2p installation docs Cc:
Parent Tickets: #1678

Description

solution for #1678:
replace commons-logging.jar in the <i2p install>/lib directory
with <i2p.i2p checkout>/apps/jetty/apache-tomcat-deployer/lib/tomcat-juli.jar

the classes in tomcat-juli.jar are needed by the jasper-runtime.jar used by routerconsole.

problem caused by commons-logging:
the commons-logging.jar causes the class not found exception (pointing at log4j) when enabling imap in bote.

commons-logging is known for its custom class loading issues/features.
background reading with many references: http://articles.qos.ch/thinkAgain.html )

Subtickets

Change History (23)

comment:1 Changed 3 years ago by kay

Add a subticket #1680.

comment:2 Changed 3 years ago by kay

Add a subticket #1678.

comment:3 Changed 3 years ago by kay

  • Keywords i2p installation needs_review added

comment:4 Changed 3 years ago by kay

  • Status changed from new to testing

comment:5 Changed 3 years ago by str4d

  • Status changed from testing to needs_work

Please do not change tickets to testing until the fix has actually been implemented.

comment:6 Changed 3 years ago by kay

1) The open task is to verify that other plugins do not also depend on commons-logging
and would have to be updated when making the change.
I suggest adding the results for the other plugins to this ticket,
and creating subtickets for those that need some tweaking.

2) There is another solution to this issue, which I would advice against,
but it should be discussed:

If the log4j jar is also added to the <i2p install>/lib directory, Bote's Imapserver will work too.

Cons:
Another logging dependency that is not really necessary for i2p.
And, all the hassle of maintaining it in future.
Other plugins would still need to tested with the new setting, b/c
it could cause other dependency troubles (different versions etc.).

Pros:
Only one added dependency.

PS:
@str4d: thanks for pointing out the use of "testing" status and changing the respective issues!

comment:7 Changed 3 years ago by zzz

  • Component changed from router/general to apps/jetty

The "think again' link above is interesting. I too have seen trouble caused by commons-logging, so removing it may be a plus.

Actually (see apps/jetty/build.xml) the commons-logging.jar that we ship is a combination of tomcat-juli.jar and the original commons-logging.jar (from installer/lib/lanunch4j/lib/)

I did test the proposal briefly (replacing the commons-logging.jar with the tomcat-juli.jar) and Jetty seemed happy. I guess Jetty switched from commons logging to Juli some time long ago? Note that commons-logging has been bundled forever, probably since we bundled jetty, and we've upgraded Jetty several times over the years. Guess we didn't notice. We may have to research when Jetty stopped requiring it. We do include Jetty in the updater (for quite a while) so we should expect anybody on the latest version to have the latest 8.1.x Jetty. I think. If not we may want to bundle this change with Jetty 9 #1512

The file would still have to be called commons-logging.jar so it overwrites the old one on an update. One benefit is the jar would be 38KB smaller. Also note that we don't currently include commons-logging in the updater, we'd have to do that forever (32 KB).

Moving component to Jetty as that's why logging was in there in the first place.

commons-logging.jar is also referenced in the build.xml files of console, i2ptunnel, susidns, and i2psnark. To be reviewed.

comment:8 Changed 3 years ago by kay

Thanks for looking into this and for naming the plugins!

I think jetty is fine without the juli.jar? The classes in tomcat-juli.jar are needed by the jasper-runtime.jar used by routerconsole (and probably others, didn't check).

Maybe, just replace commons-logging.jar with a plain text file in order to overwrite the
dependency completely (my jetty didn't complain about this).
I also noticed the juli-classes in the commons-logging jar.
But maintenance might be easier in future when jars contain what their name claims?

So replace commons-logging.jar with a simple text file and add the tomcat-juli.jar?
Or, after looking at the plugins the necessary dependency could be moved to the respective web-apps?
That would be really clean and avoid future class loading issues.
But is, of course, more work. It could be done step by step though.

It needs some more thinking ...

comment:9 Changed 3 years ago by zzz

I didn't name any plugins.

Re: naming, the horse was out of the barn on that long ago. Nothing contains what its name is anymore. And there are a few "empty" jars also. See apps/jetty/build.xml.

comment:10 Changed 3 years ago by kay

Found time to do a little survey (detailed results below).

In total, it seems the suggestion by zzz (cf. comment:7 above) to let
commons-logging.jar only contain the org.apache.juli classes will work.
And, provide the quickest way to enable the attached child issues.

Please review and fill in the aps/plugins/modules I missed.


A small 'survey' using jdeps on classes and find on sources and build.xmls
yields the following dependency results:

i2p.i2p

susidns.war,
i2ptunnel.war,
and routerconsole.war
need jasper-runtime.jar, which uses org.apache.juli.logging

i2psnark.war does not depend on commons-logging.jar and builds and runs fine without (after removing the reference from build.xml).
No other refernce/import of commons-logging (or apache.juli) found in i2p.i2p/apps/

plugins from monotone

i2pBote.war -> jasper-runtime.jar -> org.apache.juli.logging

The following plugins do not have a build reference to the commons-logging.jar nor a commons-logging import,
or run in a seperate jvm.
i2p.plugins.base
i2p.plugins.i2pcontrol
i2p.plugins.netdbtool (is this still active?)
i2p.plugins.snowman
i2p.plugins.bcprovider
i2p.plugins.i2phex
i2p.plugins.orchid
i2p.plugins.tahoe-lafs-controller
i2p.plugins.desktopgui
i2p.plugins.jIRCii
i2p.plugins.pebble
i2p.plugins.zzzot

Seedless (rev 3bd8176cac0a49c6d3e4b056531170ce41a075e1 by sponge@… 2012/08/10)

SeedlessServer?, SeedlessConsole?, and EepAnnouncer reference commons-logging.jar in their build.xml,
but they also reference jasper-runtime.jar, which actually depends on the apache-juli classes.
Their sources and other libs do not need commons-logging (afaics).
Thus, removing commons-logging from the i2p-install lib directory shouldn't do damage.

comment:11 Changed 3 years ago by str4d

Remove a subticket #1678.

comment:12 Changed 3 years ago by str4d

  • Parent Tickets set to 1678

comment:13 Changed 3 years ago by str4d

Remove a subticket #1680.

comment:14 Changed 3 years ago by str4d

  • Keywords docs added; needs_review removed

As part of this we will need to update the classpath section of the plugin spec. We currently say that "all these files in $I2P/libs can be assumed to be present", but don't specify at all which of the standard-looking JARs are not in fact standard. If this is documented somewhere else we should reference it; if it isn't, the plugin spec is as good a place as any.

comment:15 Changed 3 years ago by zzz

  • Milestone changed from undecided to 0.9.24

Removed commons-logging classes from commons-logging.jar in ee8c3ab82e0e0b6745178b9aac6006ced5f0d9f3 i2p.i2p.zzz.test2 to be propped for 0.9.24

comment:16 Changed 3 years ago by zzz

Got a report:


Error starting plugin i2pbote: java.lang.NoClassDefFoundError?: org/apache/commons/logging/LogFactory
hmmm.... I may need to ditch HH's version an install from the plugins site...
for reference, I have HH's version 0.2.10-b158


Not sure why the old version needed it, but it apparently does. Is it acceptable to require people to upgrade to the str4d bote version (and by that of course we mean delete and replace)? Or do we revert this change?

comment:17 Changed 3 years ago by str4d

My release upgraded a bunch of the bundled libraries, because everyone would need to reinstall. For example, in 50c1f1d14b2923458e795e6f8e941d0a64844b31 I upgraded the bundled James Server v3.0.0 beta 5 (used for IMAP) from the 7/22/2014 snapshot to the 6/27/2015 snapshot.

Without seeing the full error log, I can only assume that one of the libs I upgraded previously had a dependency on commons-logging but no longer does. It is more than likely the James Server libs, because it was their upgrade that caused the IMAP issue in #1678 in the first place.

There are three options here:

  1. Leave things as-is, and add a news entry to 0.9.24 informing people that they need to upgrade to my I2P-Bote version. I would re-enable IMAP support (and set min-i2p-version).
  2. Revert the change, so that HH's I2P-Bote continues to work, and my version continues to have no IMAP support.
  3. Figure out how to handle the commons-logging classpath issues properly. Then both versions would work, and I re-enable IMAP support.

IMHO there are only two reasons to still be on HH's version: a) the user hasn't noticed there is a newer version yet, or b) the user needs IMAP. Options 1) and 3) solve case b), and option 1) will make case a) users notice there is a new version. Option 2) is the status quo, which I do not want to maintain.

comment:18 Changed 3 years ago by ReturningNovice


12 07 XX:55:09.979 ERROR [I2PBoteMain ] outer.web.ConfigClientsHandler?: Error starting plugin i2pbote

java.lang.NoClassDefFoundError?: org/apache/commons/logging/LogFactory
at org.slf4j.impl.JCLLoggerFactory.getLogger(JCLLoggerFactory.java:80)
at org.slf4j.LoggerFactory?.getLogger(LoggerFactory?.java:242)
at org.slf4j.LoggerFactory?.getLogger(LoggerFactory?.java:254)
at i2p.bote.imap.ImapService?.<init>(ImapService?.java:81)
at i2p.bote.I2PBote.startImap(I2PBote.java:581)
at i2p.bote.I2PBote.startUp(I2PBote.java:405)
at i2p.bote.web.ServiceInitializer?.contextInitialized(ServiceInitializer?.java:40)
at org.eclipse.jetty.server.handler.ContextHandler?.callContextInitialized(ContextHandler?.java:782)
at org.eclipse.jetty.servlet.ServletContextHandler?.callContextInitialized(ServletContextHandler?.java:424)
at org.eclipse.jetty.server.handler.ContextHandler?.startContext(ContextHandler?.java:774)
at org.eclipse.jetty.servlet.ServletContextHandler?.startContext(ServletContextHandler?.java:249)
at org.eclipse.jetty.webapp.WebAppContext?.startContext(WebAppContext?.java:1242)
at org.eclipse.jetty.server.handler.ContextHandler?.doStart(ContextHandler?.java:717)
at org.eclipse.jetty.webapp.WebAppContext?.doStart(WebAppContext?.java:494)
at org.eclipse.jetty.util.component.AbstractLifeCycle?.start(AbstractLifeCycle?.java:64)
at net.i2p.router.web.WebAppStarter?.startWebApp(WebAppStarter?.java:60)
at net.i2p.router.web.PluginStarter?.startPlugin(PluginStarter?.java:389)
at net.i2p.router.web.ConfigClientsHandler?.startPlugin(ConfigClientsHandler?.java:579)
at net.i2p.router.web.ConfigClientsHandler?.processForm(ConfigClientsHandler?.java:106)
at net.i2p.router.web.FormHandler?.process(FormHandler?.java:263)
at net.i2p.router.web.FormHandler?.getAllMessages(FormHandler?.java:174)
at net.i2p.router.web.jsp.configclients_jsp._jspService(configclients_jsp.java:461)
at org.apache.jasper.runtime.HttpJspBase?.service(HttpJspBase?.java:70)
at javax.servlet.http.HttpServlet?.service(HttpServlet?.java:848)
at org.eclipse.jetty.servlet.ServletHolder?.handle(ServletHolder?.java:684)
at org.eclipse.jetty.servlet.ServletHandler?$CachedChain?.doFilter(ServletHandler?.java:1496)
at net.i2p.servlet.filters.XSSFilter.doFilter(XSSFilter.java:28)
at org.eclipse.jetty.servlet.ServletHandler?$CachedChain?.doFilter(ServletHandler?.java:1476)
at org.eclipse.jetty.servlet.ServletHandler?.doHandle(ServletHandler?.java:501)
at org.eclipse.jetty.server.handler.ScopedHandler?.handle(ScopedHandler?.java:137)
at org.eclipse.jetty.security.SecurityHandler?.handle(SecurityHandler?.java:557)
at org.eclipse.jetty.server.session.SessionHandler?.doHandle(SessionHandler?.java:231)
at org.eclipse.jetty.server.handler.ContextHandler?.doHandle(ContextHandler?.java:1086)
at org.eclipse.jetty.servlet.ServletHandler?.doScope(ServletHandler?.java:429)
at org.eclipse.jetty.server.session.SessionHandler?.doScope(SessionHandler?.java:193)
at org.eclipse.jetty.server.handler.ContextHandler?.doScope(ContextHandler?.java:1020)
at org.eclipse.jetty.server.handler.ScopedHandler?.handle(ScopedHandler?.java:135)
at org.eclipse.jetty.server.handler.HandlerWrapper?.handle(HandlerWrapper?.java:116)
at net.i2p.router.web.LocaleWebAppHandler?.handle(LocaleWebAppHandler?.java:100)
at org.eclipse.jetty.server.handler.ContextHandlerCollection?.handle(ContextHandlerCollection?.java:255)
at org.eclipse.jetty.server.handler.HandlerCollection?.handle(HandlerCollection?.java:154)
at org.eclipse.jetty.server.handler.HandlerWrapper?.handle(HandlerWrapper?.java:116)
at org.eclipse.jetty.server.Server.handle(Server.java:370)
at org.eclipse.jetty.server.AbstractHttpConnection?.handleRequest(AbstractHttpConnection?.java:494)
at org.eclipse.jetty.server.AbstractHttpConnection?.content(AbstractHttpConnection?.java:982)
at org.eclipse.jetty.server.AbstractHttpConnection?$RequestHandler?.content(AbstractHttpConnection?.java:1043)
at org.eclipse.jetty.http.HttpParser?.parseNext(HttpParser?.java:865)
at org.eclipse.jetty.http.HttpParser?.parseAvailable(HttpParser?.java:240)
at org.eclipse.jetty.server.AsyncHttpConnection?.handle(AsyncHttpConnection?.java:82)
at org.eclipse.jetty.io.nio.SelectChannelEndPoint?.handle(SelectChannelEndPoint?.java:696)
at org.eclipse.jetty.io.nio.SelectChannelEndPoint?$1.run(SelectChannelEndPoint?.java:53)
at org.eclipse.jetty.util.thread.QueuedThreadPool?.runJob(QueuedThreadPool?.java:608)
at org.eclipse.jetty.util.thread.QueuedThreadPool?$3.run(QueuedThreadPool?.java:543)
at java.lang.Thread.run(Unknown Source)
Caused by: java.lang.ClassNotFoundException?: org.apache.commons.logging.LogFactory?
at java.net.URLClassLoader$1.run(Unknown Source)
at java.net.URLClassLoader$1.run(Unknown Source)
at java.security.AccessController?.doPrivileged(Native Method)
at java.net.URLClassLoader.findClass(Unknown Source)
at org.eclipse.jetty.webapp.WebAppClassLoader?.loadClass(WebAppClassLoader?.java:421)
at org.eclipse.jetty.webapp.WebAppClassLoader?.loadClass(WebAppClassLoader?.java:383)
... 54 more


comment:19 Changed 3 years ago by str4d

Thanks, that confirms my suspicions in comment:17.

comment:20 Changed 3 years ago by zzz

bump @str4d need to make a final decision and get the word out well in advance of 0.9.24 release, i.e. now. I'm fine with option 1 but my opinion doesn't matter much, this is up to you as the Bote dev.

Once you decide, please do announcements on forum.i2p and in console news.

comment:21 Changed 3 years ago by str4d

Thanks for the reminder. I'm also fine with option 1, we have to get old users off the old version somehow. My only concern is that if there is a significant fraction of the net on the old version, we suddenly lose most of the network (it's still only a few hundred nodes). But without a working option 3, it's really the only workable solution.

Or as an alternative, we could revert the change for this release, and give users more time to spot the issue. But I'd think that users would be more likely to read the news if they notice something has broken :P

comment:22 Changed 3 years ago by zzz

Updated plugin spec in 4015a7e647e04c7c8f5ac446965e2627800205e0 in resolution of comment 14 above.

comment:23 Changed 3 years ago by zzz

  • Resolution set to fixed
  • Status changed from needs_work to closed

Fixed in 0.9.24 as described in comment 15 above.

Note: See TracTickets for help on using tickets.