Re: Unregistering the driver from DriverManager

Hello,

I'm trying out the PR branch just now. Seems fine (starts as a bundle,
except for remark below, connection available, can use new features such as
connection.getSchema() => "$user") ; a few small remaining observations:

- The dependency on org.osgi.service.jdbc has "resolution:=optional" in
the MANIFEST.MF file, but it should almost certainly *not* be optional (as
this causes java.lang.NoClassDefFoundError:
org/osgi/service/jdbc/DataSourceFactory).

- In the PGBundleActivator class, for stop-restart cases, the "start"
method should almost certainly have a small conditional block, such as:

if (!Driver.isRegistered()) { Driver.register(); }

=> this would mirror the deregistration in the "stop" method, and avoid
breaking code that might rely on the current static initialization behavior.

I'm still building from the forked PR repository ; from your other
messages, I assume this will soon be merged into the main "pgjdbc"
repository.

--
Christopher

On 4 January 2015 at 17:37, Alexis Meneses <alexis(dot)meneses(at)gmail(dot)com> wrote:

> Thanks for your feedback. All your observations make sense to me and I
> updated the PR branch accordingly.
>
> Alexis
>
> 2015-01-03 15:30 GMT+01:00 Christopher BROWN <brown(at)reflexe(dot)fr>:
>
>> Hi,
>>
>> I've cloned your pull request locally (haven't forked it, even although
>> I've got a github account and could do so...).
>>
>> I don't (yet) have time to try it out fully (won't be able to do so until
>> Tuesday at the earliest), but here's some initial observations.
>>
>> 1/ You might want to use a more recent version of "bnd" (you're using
>> 1.5, the current is 2.4)
>>
>> 2/ You refer to Bundle-Activator: org.postgresql.osgi.PGBundleActivator
>> in the manifest, and the source code is there, but it's not included in the
>> resulting OSGi ".jar" (generated in "/jars").
>>
>> 3/ The changes to the Driver class (register(), unregister(), and
>> isRegistered()) look good in source code.
>>
>> 4/ In PGBundleActivator::start, you should perhaps use:
>>
>> Dictionary<String,Object> properties = new Hashtable<>(4);
>> // instead of "Properties", to avoid implying that values are strings
>> (this isn't the case here)
>>
>> 5/ In PGBundleActivator::start, shouldn't "Postgresql" be written
>> "PostgreSQL" in the OSGI_JDBC_DRIVER_NAME property?
>>
>> 6/ In PGBundleActivator::stop, you should set the _registration instance
>> to null after unregistering because it's possible to restart a stopped
>> bundle.
>>
>> 7/ As you're using Bundle-ManifestVersion: 2, you might also want to
>> add Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.7))"
>> (where 1.7 is derived from "ant.java.version" during build), as this
>> enforces the JDBC 4.1 "level" indirectly by requiring the appropriate
>> JavaSE version).
>>
>> The choice of the DataSourceFactory API seems like a good idea to me, as
>> opposed to just registering the "Driver" interface directly.
>>
>> Is the 9.4 branch stable (as in safe to use as-is, even if not at
>> "feature freeze" yet)?
>>
>> --
>> Christopher
>>
>>
>> On 2 January 2015 at 17:08, Alexis Meneses <alexis(dot)meneses(at)gmail(dot)com>
>> wrote:
>>
>>> Christopher,
>>>
>>> (1) Would you mind testing if PR #241
>>> <https://github.com/pgjdbc/pgjdbc/pull/241> on Github fits your needs
>>> and works in the OSGi environment you're using?
>>> Note that it don't check presence of BundleContext via Class.forName()
>>> because this class could be erroneously present in the classpath even
>>> without an OSGi Framework being started.
>>>
>>> (2) Driver.deregister() method (named after DriverManager method) has
>>> been exposed so as to be used in other contexts like Java EE.
>>>
>>> Alexis
>>>
>>>
>>> 2014-12-29 14:17 GMT+01:00 Christopher BROWN <brown(at)reflexe(dot)fr>:
>>>
>>>> There are two main situations where it would be useful to automatically
>>>> unregister the driver:
>>>>
>>>> 1) OSGi - and the suggestion of using BundleActivator.stop() would be a
>>>> good fit here (as long as care is taken to ensure resolution=optional for
>>>> other dependencies)
>>>>
>>>> 2) Java EE web applications, using a ServletContextListener, perhaps
>>>> using annotations as described in
>>>> https://blogs.oracle.com/swchan/entry/servlet_3_0_annotations (but
>>>> this would exclude older application servers)
>>>>
>>>> With regards to (2), I generally place JDBC drivers in the main
>>>> classloader of the application server, as opposed to embedding in
>>>> WEB-INF/lib when working with webapps. Also, not all of the webapps I have
>>>> to deal with (from time to time, it's not my main focus) are up to Servlet
>>>> 3.0, many as still stuck on 2.5. And in any case, embedding JDBC drivers
>>>> in webapps (without matching versions) then accessing them via
>>>> DriverManager is may cause class lookup problems.
>>>>
>>>> A good solution to (1) above to me would be like this then (building on
>>>> the suggestion of Alexis):
>>>>
>>>> - keep the static block in driver
>>>> - check -- via Class.forName("org.osgi.framework.BundleContext") -- if
>>>> OSGi classes are visible, implying that the driver has been loaded as a
>>>> bundle, and skip the DriverManager registration in the static block (unless
>>>> forced via a system property, just in case something breaks for someone
>>>> relying on current behavior)
>>>> - register the driver in DriverManager in the BundleActivator.start()
>>>> method
>>>> - unregister it (same instance, kept as a reference) in the
>>>> BundleActivator.stop() method
>>>>
>>>> The only reason I mention (2) is because it might be useful to share
>>>> some common code. Or not. In any case, (1) is the only requirement at
>>>> this time and (2) isn't as much of a problem.
>>>>
>>>> --
>>>> Christopher
>>>>
>>>>
>>>> On 29 December 2014 at 13:45, Alexis Meneses <alexis(dot)meneses(at)gmail(dot)com>
>>>> wrote:
>>>>
>>>>> If the only concern is OSGi environments, I think that unregistering
>>>>> could be handled in a BundleActivator
>>>>> <http://www.osgi.org/javadoc/r4v43/core/org/osgi/framework/BundleActivator.html>.stop()
>>>>> implementation bundled with the driver.
>>>>>
>>>>> See pending issue #71 <https://github.com/pgjdbc/pgjdbc/issues/71> on
>>>>> Github.
>>>>>
>>>>>
>>>>> 2014-12-29 12:48 GMT+01:00 Dave Cramer <pg(at)fastcrypt(dot)com>:
>>>>>
>>>>>> I have no objection to an unregister static method being added. It's
>>>>>> not in the API so it would not effect anything really
>>>>>>
>>>>>> Dave Cramer
>>>>>>
>>>>>> dave.cramer(at)credativ(dot)ca
>>>>>> http://www.credativ.ca
>>>>>>
>>>>>> On 29 December 2014 at 04:53, Christopher BROWN <brown(at)reflexe(dot)fr>
>>>>>> wrote:
>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> I'm starting to integrate the Postgresql JDBC driver into an OSGi
>>>>>>> environment, as an OSGi bundle. I'm evaluating the different ways to avoid
>>>>>>> a classloader leak with DriverManager when hot-swapping the driver bundle
>>>>>>> without restarting the host application, and am seeking suggestions on best
>>>>>>> practice regarding the Postgresql JDBC driver.
>>>>>>>
>>>>>>> Another bundle (which I provide, it's not third-party) will directly
>>>>>>> depend upon it (loading classes directly, namely org.postgresql.Driver);
>>>>>>> when the Postgresql JDBC driver classes are loaded, the other bundle will
>>>>>>> create a DataSource using a JDBC connection pool, and register the
>>>>>>> DataSource as an OSGi service. Normally, that's all that will happen
>>>>>>> during the application lifecycle, but in principle, it's possible for the
>>>>>>> administrator to want to replace say the 9.3 driver with the 9.4 driver by
>>>>>>> removing the 9.3 ".jar" at runtime, and replacing it with the 9.4 ".jar",
>>>>>>> all at runtime; when the first ".jar" is deleted, the dependent bundle is
>>>>>>> knocked offline, unregistering the DataSource automatically, and notifying
>>>>>>> all clients; when the second is installed, the application is once again
>>>>>>> fully-functional (and all this normally occurs within a few hundred
>>>>>>> milliseconds).
>>>>>>>
>>>>>>> Looking at the source code, I can see that the org.postgresql.Driver
>>>>>>> class registers itself in a "static" block with DriverManager (which is the
>>>>>>> correct behavior regarding the JDBC spec). However, short of a brute-force
>>>>>>> loop -- like this one: http://stackoverflow.com/a/5315467 (enhanced
>>>>>>> to check the class name of each driver, to avoid clobbering unrelated
>>>>>>> driver registrations) -- is there any other approach possible or that could
>>>>>>> be added, say a NonRegisteringDriver (superclass of Driver, with all logic
>>>>>>> except for the static initializer) or an "unregister()" static method, or a
>>>>>>> field containing the registered Driver instance?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Christopher
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>