DriverManagerCleanUp can't really work for many scenarios #80

Geker · 2018-04-19T03:22:05Z

DriverManager.getDrivers() only return the drivers which be load by caller(DriverManagerCleanUp.class).
For many scenarios the Caller is not the same classloader which load the jdbc Drivers. So ,DriverManager.getDrivers() will return empty driver list,and DriverManagerCleanUp do nothing.
I think it need get registeredDrivers by reflection.

Add getAllDrivers method. DriverManager.getDrivers() only return the drivers which be load by caller(DriverManagerCleanUp.class). For many scenarios the Caller is not the same classloader which load the jdbc Drivers

mjiderhamn · 2018-04-19T06:03:09Z

Could you please adhere to the coding standards of the project, such as indentation with 2 spaces, camel humping etc?

mjiderhamn · 2018-04-19T06:03:30Z

...ore/src/main/java/se/jiderhamn/classloader/leak/prevention/cleanup/DriverManagerCleanUp.java

+ * Deregister JDBC drivers loaded by classloader
+ *
+ *


Why these empty JavaDoc lines?

sorry I use the diffenrent Java code style template ,i shoud follow your project coding standards

mjiderhamn · 2018-04-19T06:04:07Z

...ore/src/main/java/se/jiderhamn/classloader/leak/prevention/cleanup/DriverManagerCleanUp.java

+            preventor.warn("get All registeredDrivers Exception ");
+            preventor.error(e);
+


Can't we default to DriverManager.getDrivers() in case reflection fails?

mjiderhamn · 2018-04-19T06:05:09Z

...ore/src/main/java/se/jiderhamn/classloader/leak/prevention/cleanup/DriverManagerCleanUp.java

+        catch (Exception e) {
+            preventor.warn("get All registeredDrivers Exception ");
+            preventor.error(e);


We should decide whether this makes up a warning or an error - not both

mjiderhamn · 2018-04-19T06:05:25Z

...ore/src/main/java/se/jiderhamn/classloader/leak/prevention/cleanup/DriverManagerCleanUp.java

+
+        }
+        return (result.elements());


Remove the extra parethesis

mjiderhamn · 2018-04-19T06:07:24Z

...ore/src/main/java/se/jiderhamn/classloader/leak/prevention/cleanup/DriverManagerCleanUp.java

+    @Override
+    public void cleanUp(ClassLoaderLeakPreventor preventor) {
+        final List<Driver> driversToDeregister = new ArrayList<Driver>();


Is this list even needed now that we know we won't cause ConcurrentModificationException (depending on the reflectino fall below, by all means), or can't we deregister in the first loop?

yes,driversToDeregister is no need any more.

mjiderhamn · 2018-04-19T06:08:06Z

...ore/src/main/java/se/jiderhamn/classloader/leak/prevention/cleanup/DriverManagerCleanUp.java

+        while (allDrivers.hasMoreElements()) {
+            final Driver driver = allDrivers.nextElement();
+            if (preventor.isLoadedInClassLoader(driver)) // Should be true for all returned by DriverManager.getDrivers()


Is the comment still relevant?

mjiderhamn · 2018-04-19T06:09:18Z

...ore/src/main/java/se/jiderhamn/classloader/leak/prevention/cleanup/DriverManagerCleanUp.java

+     * @param preventor
+     * @return
+     */


Please clarify the JavaDoc

mjiderhamn · 2018-04-19T06:11:36Z

...ore/src/main/java/se/jiderhamn/classloader/leak/prevention/cleanup/DriverManagerCleanUp.java

+     * @return
+     */
+    public java.util.Enumeration<Driver> getAllDrivers(ClassLoaderLeakPreventor preventor) {


Why Enumeration rather than Collection?

DriverManager.getDrivers() return type is Enumeration

mjiderhamn · 2018-04-19T06:12:07Z

...ore/src/main/java/se/jiderhamn/classloader/leak/prevention/cleanup/DriverManagerCleanUp.java

+     */
+    public java.util.Enumeration<Driver> getAllDrivers(ClassLoaderLeakPreventor preventor) {
+        java.util.Vector<Driver> result = new java.util.Vector<Driver>();


Why Vector rather than ArrayList? Synchronization cannot possibly be needed.

because method DriverManager.getDrivers() use Vector,So i follow the usage

mjiderhamn · 2018-04-19T06:13:25Z

...ore/src/main/java/se/jiderhamn/classloader/leak/prevention/cleanup/DriverManagerCleanUp.java

+        java.util.Vector<Driver> result = new java.util.Vector<Driver>();
+        try {
+            CopyOnWriteArrayList<?> driverinfos = preventor.getStaticFieldValue(DriverManager.class, "registeredDrivers");


I suggest using List (or even Collection) as the variable type, and by all means keep CopyOnWriteArrayList as a comment. That way this class still works even if the internals of DriverManager is updated to use something other than CopyOnWriteArrayList

mjiderhamn · 2018-04-19T06:15:58Z

What is the scenario we're trying to solve here? The leak prevention library being part of a framework/application server, while the JDBC driver is dynamically loaded in an application instance (.war file)?

Geker · 2018-04-19T07:11:57Z

Yes I have a app server. which have two level classloader. the server classloader load classloader-leak-prevention.jar .The server main-thread execute: runPreClassLoaderInitiators -> start war classloader （war-classloader load jdbc drivers / .war etc）->runCleanUps

I modify the code, please see the code again.

modify code style; return DriverManager.getDrivers() if Exception occurred

mjiderhamn · 2018-04-23T18:42:14Z

...ore/src/main/java/se/jiderhamn/classloader/leak/prevention/cleanup/DriverManagerCleanUp.java

+   * @param preventor
+   * @return All drivers in DriverManager's registeredDrivers field,or
+   *         DriverManager.getDrivers() if exception occurred
+   */


Please clean up the JavaDoc a bit. (For example, you don't need to document method X with "Add X method". Either add documentation or remove @param)

mjiderhamn · 2018-04-23T18:43:55Z

...ore/src/main/java/se/jiderhamn/classloader/leak/prevention/cleanup/DriverManagerCleanUp.java

+      }
+    } catch (Exception e) {
+      preventor.warn("get All registeredDrivers Exception");
+      return DriverManager.getDrivers();


The code now assumes the returned Enumeration does not cause ConcurrentModificationException if driver is removed while looping. Is that guaranteed by the API contract, or should we make of copy to ensure the library does not start failing in case the DriverManager internals happens to be changed?

I think it's no reason the jdk developer to changed the default manner of getDrivers. they are already considered avoid ConcurrentModificationException in current code.

mjiderhamn · 2018-04-23T18:44:17Z

...ore/src/main/java/se/jiderhamn/classloader/leak/prevention/cleanup/DriverManagerCleanUp.java

    }
+    return result.elements();


Move this inside try (unless used for copying, see below)

mjiderhamn · 2018-04-23T18:44:39Z

...ore/src/main/java/se/jiderhamn/classloader/leak/prevention/cleanup/DriverManagerCleanUp.java

+   *         DriverManager.getDrivers() if exception occurred
+   */
+  public Enumeration<Driver> getAllDrivers(ClassLoaderLeakPreventor preventor) {
+    Vector<Driver> result = new java.util.Vector<Driver>();


Move this inside try (unless used for copying, see below)

OlegYch · 2018-04-24T10:49:32Z

DriverManager.deregisterDriver needs to be done via reflection too as it also checks caller classloader

mjiderhamn · 2018-04-24T15:44:32Z

Good catch @OlegYch !
Add some more work, though.

Geker · 2018-05-02T08:03:55Z

@OlegYch thanks your advices.
I'm already change the code.

Summary: In java 9, sun.misc.GC is moved to sun.rmi.transport.GC , trying again if sun.misc.GC is not found else throwing ClassNotFoundException.

Conflicts: README.md classloader-leak-prevention/classloader-leak-prevention-core/README.md

Conflicts: classloader-leak-prevention/classloader-leak-prevention-core/pom.xml classloader-leak-prevention/classloader-leak-prevention-servlet/pom.xml classloader-leak-prevention/classloader-leak-prevention-servlet3/pom.xml classloader-leak-prevention/pom.xml

This reverts commit 6a13eff.

Conflicts: classloader-leak-prevention/classloader-leak-prevention-core/pom.xml classloader-leak-prevention/classloader-leak-prevention-servlet/pom.xml classloader-leak-prevention/classloader-leak-prevention-servlet3/pom.xml classloader-leak-prevention/pom.xml

...ore/src/main/java/se/jiderhamn/classloader/leak/prevention/cleanup/DriverManagerCleanUp.java

…evention.git

Geker · 2019-01-04T09:36:22Z

I accept your suggestion,please review again.
The check failure was not caused by my commit.

Geker added 2 commits April 19, 2018 10:40

modify DriverManagerCleanUp

2a4af57

Add getAllDrivers method. DriverManager.getDrivers() only return the drivers which be load by caller(DriverManagerCleanUp.class). For many scenarios the Caller is not the same classloader which load the jdbc Drivers

refactor DriverManagerCleanUp

899feb3

Geker closed this Apr 19, 2018

Geker reopened this Apr 19, 2018

mjiderhamn reviewed Apr 19, 2018

View reviewed changes

refactor code

27e200d

modify code style; return DriverManager.getDrivers() if Exception occurred

mjiderhamn reviewed Apr 23, 2018

View reviewed changes

Geker added 6 commits April 28, 2018 18:25

bugfix reflection to do the DriverManager.deregisterDriver

3d91e47

bugfix:Do the work as DriverManager.deregisterDriver by reflection

4dcd5c0

bugfix:Do the work as DriverManager.deregisterDriver by reflection

06555f4

refactor import

fdfad58

Code changed to compatibile JDK7

f5400e7

Code changed to compatibile JDK7

3dd4b4b

Chiranjivee Thakur and others added 7 commits May 31, 2018 09:50

Fix Init sun.misc.GC for Java 9

516dfbf

Summary: In java 9, sun.misc.GC is moved to sun.rmi.transport.GC , trying again if sun.misc.GC is not found else throwing ClassNotFoundException.

2.6.1 released, updating README.md

64e2cac

Conflicts: README.md classloader-leak-prevention/classloader-leak-prevention-core/README.md

Revert "[maven-release-plugin] prepare for next development iteration"

60fbbac

This reverts commit 6a13eff.

更新readme

9474472

Merge branch 'master' into master

6c10c17

mjiderhamn reviewed Dec 15, 2018

View reviewed changes

Geker added 2 commits January 4, 2019 17:33

modify DriverManagerCleanUp by suggestion

1233a32

Merge branch 'master' of https://github.com/Geker/classloader-leak-pr…

c69a6b4

…evention.git

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DriverManagerCleanUp can't really work for many scenarios #80

DriverManagerCleanUp can't really work for many scenarios #80

Geker commented Apr 19, 2018

mjiderhamn commented Apr 19, 2018 •

edited

Loading

mjiderhamn Apr 19, 2018

Geker Apr 19, 2018

mjiderhamn Apr 19, 2018

mjiderhamn Apr 19, 2018

mjiderhamn Apr 19, 2018

mjiderhamn Apr 19, 2018

Geker Apr 19, 2018 •

edited

Loading

mjiderhamn Apr 19, 2018

mjiderhamn Apr 19, 2018

mjiderhamn Apr 19, 2018

Geker Apr 19, 2018

mjiderhamn Apr 19, 2018

Geker Apr 19, 2018

mjiderhamn Apr 19, 2018

mjiderhamn commented Apr 19, 2018

Geker commented Apr 19, 2018 •

edited

Loading

mjiderhamn Apr 23, 2018

mjiderhamn Apr 23, 2018

Geker May 2, 2018

mjiderhamn Apr 23, 2018 •

edited

Loading

mjiderhamn Apr 23, 2018

OlegYch commented Apr 24, 2018

mjiderhamn commented Apr 24, 2018

Geker commented May 2, 2018 •

edited

Loading

Geker commented Jan 4, 2019 •

edited

Loading

		preventor.warn("get All registeredDrivers Exception ");
		preventor.error(e);

DriverManagerCleanUp can't really work for many scenarios #80

Are you sure you want to change the base?

DriverManagerCleanUp can't really work for many scenarios #80

Conversation

Geker commented Apr 19, 2018

mjiderhamn commented Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Geker Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjiderhamn commented Apr 19, 2018

Geker commented Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjiderhamn Apr 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlegYch commented Apr 24, 2018

mjiderhamn commented Apr 24, 2018

Geker commented May 2, 2018 • edited Loading

Geker commented Jan 4, 2019 • edited Loading

mjiderhamn commented Apr 19, 2018 •

edited

Loading

Geker Apr 19, 2018 •

edited

Loading

Geker commented Apr 19, 2018 •

edited

Loading

mjiderhamn Apr 23, 2018 •

edited

Loading

Geker commented May 2, 2018 •

edited

Loading

Geker commented Jan 4, 2019 •

edited

Loading