Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 358263

Summary: JDBCSessionIdManager should not use Class.forName()
Product: [RT] Jetty Reporter: Rüdiger Herrmann <ruediger.herrmann>
Component: buildAssignee: Jesse McConnell <jesse.mcconnell>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: janb, jetty-inbox
Version: 7.4.0Flags: jesse.mcconnell: indigo-
Target Milestone: ---   
Hardware: All   
OS: All   
Whiteboard:
Bug Depends on:    
Bug Blocks: 358557    

Description Rüdiger Herrmann CLA 2011-09-20 11:29:03 EDT
Jetty 7 and 8 use Class.forName() to load the JDBC driver in the initializeDatabase() method of JDBCSessionIdManager This approach fails when running in OSGi and currently prevents us from using Jetty embedded in OSGi. Apart from not working with OSGi, Class.forName() bears further issues (see [1] for a discussion).
I propose to change to way the database driver is loaded. If there was a method
  setDriverInfo(Class<? extends> java.sql.Driver driverClass, String connectionUrl ) 
clients could load the driver themselves and pass it in.
Further options to avoid Class.forName() can be found here [2].

[1] http://blog.bjhargrave.com/2007/09/classforname-caches-defined-class-in.html
[2] http://njbartlett.name/2010/08/30/osgi-readiness-loading-classes.html
Comment 1 Jesse McConnell CLA 2011-09-20 15:10:42 EDT
fixed, I added a setDriverInfo(Driver, String) method and the initializeDatabase() method now checks to see if a driver has been set and calls DriverManager.registerDriver(_driver) on it.
Comment 2 Rüdiger Herrmann CLA 2011-09-20 18:42:34 EDT
(sorry to pollute this bug, but I couldn't find any info)
When will the p2 repository be updated with this change? Is there a p2 repo with nightlies?
Comment 3 Jesse McConnell CLA 2011-09-26 10:10:40 EDT
you need for jetty 7 or jetty 8?

we probably need to setup a hudson job so we can push out an 8 repo nightly as well
Comment 4 Rüdiger Herrmann CLA 2011-09-26 15:36:37 EDT
(In reply to comment #3)
> you need for jetty 7 or jetty 8?
We want to migrate our integration tests to Jetty 8 (bug 358253), so Jetty 8 is what we need (plus a workaround or solution to bug 358251)
Comment 5 Rüdiger Herrmann CLA 2011-10-04 13:31:03 EDT
Sorry to bother you again, but is there any download of Jetty 8 bundles available that include this fix?
Comment 6 Jesse McConnell CLA 2011-10-04 14:20:41 EDT
Jan did the merge and it should be pushed into the nightly tonight...though I will run that job right now for you.

https://hudson.eclipse.org/hudson/view/Jetty-RT/job/jetty-rt-bundles-8/17/

When that job is done the development location will be updated.

I got all this working yesterday for you btw :)

you should be able to get it from

http://download.eclipse.org/jetty/updates/jetty-bundles-8.x/development

that is the nightly

cheers,
jesse
Comment 7 Rüdiger Herrmann CLA 2011-10-04 16:21:43 EDT
(In reply to comment #6)
> Jan did the merge and it should be pushed into the nightly tonight...though I
> will run that job right now for you.
> 
> https://hudson.eclipse.org/hudson/view/Jetty-RT/job/jetty-rt-bundles-8/17/
> 
> When that job is done the development location will be updated.
> 
> I got all this working yesterday for you btw :)
> 
> you should be able to get it from
> 
> http://download.eclipse.org/jetty/updates/jetty-bundles-8.x/development
> 
> that is the nightly
> 
> cheers,
> jesse
With the bundles from 8.x development everything works fine. Thanks a lot Jesse!
Comment 8 Rüdiger Herrmann CLA 2011-10-12 12:59:00 EDT
Sorry to reopen, but providing setDriverInfo(Driver,String) is only half the battle. When searching for a suitable driver, the code in DriverManager#getConnection() checks whether the driver was loaded with the same class loader as the caller of #getConnection(). If this isn't the case the driver is skipped.

In OSGI, this is never the case when an application bundle loads the driver and calls setDriverInfo(Driver,String) and later on Jetty (in a bundle of its own) calls DriverManager#getConnection().

If there was a setDataSource(DataSource) instead, the application bundle could provide an implementation that registers the driver and calls DriverManager#getConnection() from the same class loader.
What do you think?
Comment 9 Jan Bartel CLA 2011-10-12 23:25:46 EDT
Ruediger,

I've added a setDatasource(DataSource) method onto JDBCSessionIdManager. I'm in the process of pushing a snapshot, should be up there soon:

http://oss.sonatype.org/content/groups/jetty/org/eclipse/jetty/jetty-distribution/

If you could test it out, that would great.

thanks
Jan
Comment 10 Rüdiger Herrmann CLA 2011-10-13 08:37:33 EDT
(In reply to comment #9)
> Ruediger,
> I've added a setDatasource(DataSource) method onto JDBCSessionIdManager. I'm in
> the process of pushing a snapshot, should be up there soon:
> 
> http://oss.sonatype.org/content/groups/jetty/org/eclipse/jetty/jetty-distribution/
The latest I could find is a jetty-distribution-8.0.3.v20111011-1.zip. Besides I would need bundles to test. Preferrable from a p2 repository or a zip with just the bundles.
So I tried http://download.eclipse.org/jetty/updates/jetty-bundles-8.x - same here.
I am afraid, but I cannot find a version >= 2011-01-13
Comment 11 Jesse McConnell CLA 2011-10-13 08:51:04 EDT
It probably needs merged to jetty-8 and then a build triggered (or wait for the daily build)

I'll get a merge to jetty-8 done in a bit and trigger the build for you.

Note, the timestamp vs SNAPSHOT qualifier on the development builds of bundles hasn't been done yet...I am not super familiar with that tooling but its on my list.
Comment 12 Rüdiger Herrmann CLA 2011-10-13 08:54:29 EDT
(In reply to comment #11)
> I'll get a merge to jetty-8 done in a bit and trigger the build for you.
Thanks!

> Note, the timestamp vs SNAPSHOT qualifier on the development builds of bundles
> hasn't been done yet...
As far as I amconcerned, for testing locally only, the SNAPSHOT versions are sufficient.
Comment 13 Jesse McConnell CLA 2011-10-13 09:07:32 EDT
ok, developent bundles for 8 have been updated, give it a whirl and let me know if you see it so I can close the issue
Comment 14 Jesse McConnell CLA 2011-10-13 09:10:16 EDT
version is still showing 8.0.2.SNAPSHOT on there, could you confirm that the fix is in there and that this is just a build issue with our bundles build?
Comment 15 Rüdiger Herrmann CLA 2011-10-13 09:31:35 EDT
(In reply to comment #14)
> version is still showing 8.0.2.SNAPSHOT on there, could you confirm that the
> fix is in there and that this is just a build issue with our bundles build?
When installing from 
   http://download.eclipse.org/jetty/updates/jetty-bundles-8.x
the version shown in the p2 target management UI is 8.0.4, the bundles however are marked as 8.0.3.v20111011.
Comment 17 Jesse McConnell CLA 2011-10-13 09:41:43 EDT
i updated the version of the bundle tooling that generates this to 8.0.4 so artifacts ought to be named correctly now...but if I follow you correctly you saying it pulling the released 8.0.3 bundles

I am going to deploy a 8.0.4 snapshot, could be that is it
Comment 18 Rüdiger Herrmann CLA 2011-10-13 09:49:50 EDT
(In reply to comment #16)
> http://download.eclipse.org/jetty/updates/jetty-bundles-8.x/development ?

Sorry for the confusion. From this URL I get the 8.0.2SNAPSHOT bundles. However the setDataSouce() method isn't present.
Comment 19 Jesse McConnell CLA 2011-10-13 09:52:14 EDT
https://hudson.eclipse.org/hudson/view/Jetty-RT/

I am forcing a build of jetty-nightly-8 that should pickup the changes that should then flow into the jetty-rt-bundles-8 build...so if you want to watch those for then the cycle is complete it should be updated.  We'll have to check the artifacts though to make sure they got picked up.  I profess that much of this osgi build process is a black box :/
Comment 20 Jesse McConnell CLA 2011-10-13 10:05:21 EDT
ok, give it a whirl
Comment 21 Jesse McConnell CLA 2011-10-13 10:07:34 EDT
I see a bundle version of 8.0.2.SNAPSHOT but an implementation version of 8.0.4-SNAPSHOT so....optimistic?

really need to talk to hugues about how to make this clearer
Comment 22 Rüdiger Herrmann CLA 2011-10-13 18:26:38 EDT
(In reply to comment #20)
> ok, give it a whirl
Even though it looks a bit weird now (8.0.2 bundles mixed with 8.0.4 source bundles), the target platform works. The #setDataSource() method is there and it seems to do the trick. But as it get's late right now, please give me some more time to investigate. I will report here.
Thanks for all your time and effort, just great!
Comment 23 Rüdiger Herrmann CLA 2011-10-14 09:07:32 EDT
With #setDataSource() I am able to use the JDBCSessionIdManager in OSGi.
Thanks again for your help.