Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Force loading of JDBC drivers in runtime classloader #7089

Merged
merged 1 commit into from
Feb 10, 2020
Merged

Force loading of JDBC drivers in runtime classloader #7089

merged 1 commit into from
Feb 10, 2020

Conversation

jaikiran
Copy link
Member

@jaikiran jaikiran commented Feb 8, 2020

Fixes #7079

The issue noted there is due to the way the java.sql.DriverManager deals with access to java.sql.Driver instances. Callers of various APIs on the DriverManager are allowed access to Driver based on classloader checks.

In that linked issue, it so happens that the Postgres driver gets loaded (early) in the augmentation classloader whereas the Tracing driver (the one which needs that underlying Postgres driver) gets loaded (a bit late) in the runtime classloader. As a result, the call from the Tracing driver, at runtime, is not allowed access to the Postgres driver.

The reason why this works in Java 11 is because in Java 11 the java.sql.DriverManager loads the drivers lazily[1] unlike in Java 8 where the DriverManager loads them eagerly[2]. Given the reliance on TCCL, in DriverManager for loading the drivers, this leads to potentially non-deterministic situation where the driver may or may not be loaded in the correct classloader.

To remedy this, the commit in this PR, forces loading of (any available) Drivers in the runtime classloader just before the datasource is being produced. This ensures that these drivers are then accessible in the rest of the runtime.

The commit also includes a testcase which reproduces the failure and verifies the fix.

[1] https://hg.openjdk.java.net/jdk/jdk/file/3b89be93a7e7/src/java.sql/share/classes/java/sql/DriverManager.java#l570
[2] https://hg.openjdk.java.net/jdk8u/jdk8u/jdk/file/4db0e91b95c8/src/share/classes/java/sql/DriverManager.java#l100

@stuartwdouglas
Copy link
Member

Fix looks good to me, my only real question is why the test is in the opentracing module?

@jaikiran
Copy link
Member Author

Hello @stuartwdouglas, thank you for the review.

my only real question is why the test is in the opentracing module?

I started off trying to reproduce the original issue which uses the opentracing the JDBC driver URL (which requires the opentracing extension). That way it tests the same code path of the original issue. Plus, I couldn't find any tests in our Quarkus codebase which tests the jdbc:tracing: JDBC URL format which is only mentioned in the docs https://quarkus.io/guides/opentracing#jdbc

Having said that, I think I can come up with a test case which reproduces this outside of opentracing. Let me know if you prefer that and I can work on it.

@stuartwdouglas stuartwdouglas merged commit e59e07c into quarkusio:master Feb 10, 2020
@stuartwdouglas stuartwdouglas added this to the 1.3.0 milestone Feb 10, 2020
@jaikiran jaikiran deleted the qk-7079 branch July 4, 2020 14:25
yrodiere added a commit to yrodiere/quarkus that referenced this pull request Jan 13, 2025
This was introduced in quarkusio#7089,
which was specifically about a bug when using opentracing,
which no longer has an extension in core,
and even its Quarkiverse extension is no longer maintained:
https://github.com/quarkiverse/quarkus-smallrye-opentracing

The service loading is also causing problems with quarkusio#41995

So, let's not do it all, assuming tests passes.
yrodiere added a commit to yrodiere/quarkus that referenced this pull request Jan 13, 2025
This was introduced in quarkusio#7089,
which was specifically about a bug when using opentracing,
which no longer has an extension in core,
and even its Quarkiverse extension is no longer maintained:
https://github.com/quarkiverse/quarkus-smallrye-opentracing

The service loading is also causing problems with quarkusio#41995

So, let's not do it all, assuming tests passes.
yrodiere added a commit to yrodiere/quarkus that referenced this pull request Jan 13, 2025
This was introduced in quarkusio#7089,
which was specifically about a bug when using opentracing,
which no longer has an extension in core,
and even its Quarkiverse extension is no longer maintained:
https://github.com/quarkiverse/quarkus-smallrye-opentracing

The service loading is also causing problems with quarkusio#41995

So, let's not do it all, assuming tests passes.
yrodiere added a commit to yrodiere/quarkus that referenced this pull request Jan 13, 2025
This was introduced in quarkusio#7089,
which was specifically about a bug when using opentracing,
which no longer has an extension in core,
and even its Quarkiverse extension is no longer maintained:
https://github.com/quarkiverse/quarkus-smallrye-opentracing
The bug was also specific to Java 8.

The service loading is also causing problems with quarkusio#41995

So, let's not do it all, assuming tests passes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1.3.0Alpha1 cannot initially find JDBC Driver on Java8, but works on Java11
2 participants