-
Notifications
You must be signed in to change notification settings - Fork 840
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
refactor:(loom) replace the usages of synchronized with ReentrantLock #2635
Conversation
The PR looks good to me, except there's still some Have you tried adjusting it as well? I'm inclined we might get concurrency issues if we replace only a fraction of synchronizations on |
My intention was to leave those for now and do that change as a separate PR as they are a different lock - a lock on the connection instance rather than the statement. If we want to include the change for the connection lock we could and there are a couple of things to note.
We could do this change as an additional commit to this PR or a separate PR (or even hold off for now if that was desired).
We need to specifically be careful wrt use of In this case of the lock on PgStatement [and subclasses] we need to:
So we do need to be careful but yes we can replace just the lock on PgStatement from synchronized to ReentrantLock and be very comfortable in doing that and leaving other locks like |
Suppose there's a class: class Counter {
private int value;
synchronized void increase() {
value++;
}
synchronized void decrease() {
value--;
}
} I would like to refrain from having two PRs that address class Counter {
private int value;
private Lock lock = new ReentrantLock();
void increase() {
lock.lock();
try {
value++;
} finally {
lock.unlock();
}
}
synchronized void decrease() {
value--;
}
} In that case, calling That is why I would rather prefer a single PR with multiple commits instead of multiple PRs that might result in a half-broken code. GitHub does allow clicking into individual commits that form the PR, so I believe PRs like https://github.com/GoogleCloudPlatform/app-gradle-plugin/pull/398/commits are reviewable. Of course, there are cases when the change is a no-brainer. It might be fine to include them (and even merge) in a separate PRs. However, I would refrain from merging only a half of Statement or Connection-related synchronizations because it would make |
Are we ok changing the type of See my comment from "We need to decide how to expose the PgConnection lock to PgStatement." |
By the way, @rbygrave , have you considered adding something like |
Not really no. The JDK devs could have added such support to the jdk but didn't so some hesitation from me due to that. I think there are 2 approaches ... We have a lot of lock code blocks so I can see the value of doing it (remove the finally blocks) but there does look to be a downside to both approaches as I see it. Edit: FWIW right now my gut says keep it simple and stick with the finally blocks. Slightly ugly but simple and everyone immediately understands it. Happy to try an AutoClosableLock if you or others are keen. |
Could you please clarify what you mean by "can be used incorrectly"?
I think it is a small addition that makes the code slightly easier to read/write, and it brings no downsides performance-wise. |
With B)
It's a compile error to add the Said another way, for this change we can never re-use an existing
Yes agreed, closure overhead there. |
Did that make sense @vlsi ? Are you still keen for us to create a |
The following would be a misuse of the lock API which would be impossible with lock.lock()
try {
...
} catch(SQLException e) {
e.printStacktrace(); // or whatever
}
Well, there could be a private (or package-private) method try (AutoClosable ignored = obtainLock()) {
...
} |
Yes agreed. That is what I said as well.
Pretty much but I would suggest that try (AutoClosable ignored = lock.obtainLock()) {
...
} For myself, my gut says that I'd prefer being more explicit using try (ResourceLock ignored = lock.obtainLock()) {
...
} ... as I think that would be clearer / more explicit about what's going on for folks reading the code in the future. For myself, I'm happy if we take a day or two for everyone to be comfortable with the style we want to use going forward because it's going to be repeated a lot. Of course we can change our minds later on but yeah happy to get good comfort levels with the style and approach first - a few days isn't going to hurt us. Currently our question is, are we keen to create a For me I'm happy either way. I could even make that change using a |
Hi @vlsi @davecramer and anyone else participating here. Just checking in on ... Are we keen to create a ResourceLock extends ReentrantLock implements AutoClosable as per (B)? Cheers, Rob. |
I incline to |
I'll defer to @vlsi as I am not an expert in this area |
Ok, added |
Had a chance to look at it yet? |
In general, I like how it all goes, however, there are test failures, so I did not look closely. |
I'm surprised our code style check doesn't pick up on the lack of the header in ResourceLock and ResourceLockTest |
By default it prints limited number of warnings into the console to keep the log manageable/readable: https://github.com/pgjdbc/pgjdbc/actions/runs/3279472138/jobs/5613343062#step:4:288 For instance, if someone accidentally commits "change all LF to CRLF line endings", then we don't want to get a ton of lines with wrong line endings. |
I believe this happens because of incorrect Lock API usage: we shoudn't use final ReentrantLock lock = new ReentrantLock();
final Condition lockCondition = lock.newCondition();
void foo() {
lock.lock();
try {
lockCondition.await();
// or
lockCondition.signal();
} finally {
lock.unlock();
}
} And since we need only one public final class ResourceLock extends ReentrantLock implements AutoCloseable {
private final Condition condition = this.newCondition();
// add either the getter for the condition field or even some wrapper methods, for example:
public void await() throws InterruptedException {
condition.await();
}
//...
} |
Hi, just a heads up - I've tried running some loom tests on this PR with
Full stacktrace:
|
Yes. I had a "next change" to push which includes QueryExecutorImpl, QueryExecutorBase and CopyOperationImpl. If people are happy with what we have so far I can push this as an extra commit to this PR or another separate PR. The changes here are the expected/mechanical changes + a change to If this next workflow run goes all green (style fixes for imports and checkerframework suppress warning) and if everyone is happy with it we can choose to add those changes as another commit to this PR or create a separate PR. |
This PR is ready to run again @davecramer @vlsi - I have fixed the style and checkerframework issues (and this time, I have run those all locally to ensure I got those correct touch wood) |
@vlsi nor does Heinz share the benchmark, odd. |
This MR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [flow-bin](https://github.com/flowtype/flow-bin) ([changelog](https://github.com/facebook/flow/blob/master/Changelog.md)) | devDependencies | minor | [`^0.201.0` -> `^0.203.0`](https://renovatebot.com/diffs/npm/flow-bin/0.201.0/0.203.1) | | [com.rometools:rome](https://rometools.com) ([source](https://github.com/rometools/rome)) | compile | minor | `2.0.0` -> `2.1.0` | | [org.postgresql:postgresql](https://jdbc.postgresql.org) ([source](https://github.com/pgjdbc/pgjdbc)) | build | minor | `42.5.4` -> `42.6.0` | | [com.diffplug.spotless:spotless-maven-plugin](https://github.com/diffplug/spotless) | build | minor | `2.34.0` -> `2.35.0` | | [org.apache.maven.plugins:maven-resources-plugin](https://maven.apache.org/plugins/) | build | patch | `3.3.0` -> `3.3.1` | | [io.quarkus:quarkus-maven-plugin](https://github.com/quarkusio/quarkus) | build | patch | `2.16.4.Final` -> `2.16.6.Final` | | [io.quarkus:quarkus-universe-bom](https://github.com/quarkusio/quarkus-platform) | import | patch | `2.16.4.Final` -> `2.16.6.Final` | --- ### Release Notes <details> <summary>flowtype/flow-bin</summary> ### [`v0.203.1`](flow/flow-bin@0c16b26...5e0645d) [Compare Source](flow/flow-bin@0c16b26...5e0645d) ### [`v0.203.0`](flow/flow-bin@861f798...0c16b26) [Compare Source](flow/flow-bin@861f798...0c16b26) ### [`v0.202.1`](flow/flow-bin@2b48bba...861f798) [Compare Source](flow/flow-bin@2b48bba...861f798) ### [`v0.202.0`](flow/flow-bin@86aea9c...2b48bba) [Compare Source](flow/flow-bin@86aea9c...2b48bba) </details> <details> <summary>rometools/rome</summary> ### [`v2.1.0`](https://github.com/rometools/rome/releases/tag/2.1.0) [Compare Source](rometools/rome@2.0.0...2.1.0) <!-- Release notes generated using configuration in .github/release.yml at 2.1.0 --> #### What's Changed ##### ⭐ New Features - Downgrade Java from version 11 to 8 by [@​PatrickGotthard](https://github.com/PatrickGotthard) in rometools/rome#642 - Add support for GraalVM native images by [@​artembilan](https://github.com/artembilan) in rometools/rome#636 ##### 🔨 Dependency Upgrades - Bump maven-compiler-plugin from 3.10.1 to 3.11.0 by [@​dependabot](https://github.com/dependabot) in rometools/rome#635 ##### 🧹 Cleanup - Remove unused config files by [@​PatrickGotthard](https://github.com/PatrickGotthard) in rometools/rome#632 - Polish GitHub workflows by [@​PatrickGotthard](https://github.com/PatrickGotthard) in rometools/rome#633 - Polish code by [@​antoniosanct](https://github.com/antoniosanct) in rometools/rome#631 ##### ✔ Other Changes - Update configuration for automatically generated release notes by [@​PatrickGotthard](https://github.com/PatrickGotthard) in rometools/rome#634 #### New Contributors - [@​artembilan](https://github.com/artembilan) made their first contribution in rometools/rome#636 **Full Changelog**: rometools/rome@2.0.0...2.1.0 </details> <details> <summary>pgjdbc/pgjdbc</summary> ### [`v42.6.0`](https://github.com/pgjdbc/pgjdbc/blob/HEAD/CHANGELOG.md#​4260-2023-03-17-153434--0400) ##### Changed fix: use PhantomReferences instead of `Obejct.finalize()` to track Connection leaks [MR #​2847](pgjdbc/pgjdbc#2847) The change replaces all uses of Object.finalize with PhantomReferences. The leaked resources (Connections) are tracked in a helper thread that is active as long as there are connections in use. By default, the thread keeps running for 30 seconds after all the connections are released. The timeout is set with pgjdbc.config.cleanup.thread.ttl system property. refactor:(loom) replace the usages of synchronized with ReentrantLock [MR #​2635](pgjdbc/pgjdbc#2635) Fixes [Issue #​1951](pgjdbc/pgjdbc#1951) </details> <details> <summary>diffplug/spotless</summary> ### [`v2.35.0`](https://github.com/diffplug/spotless/blob/HEAD/CHANGES.md#​2350---2023-02-10) ##### Added - CleanThat Java Refactorer. ([#​1560](diffplug/spotless#1560)) - Introduce `LazyArgLogger` to allow for lazy evaluation of log messages in slf4j logging. ([#​1565](diffplug/spotless#1565)) ##### Fixed - Allow multiple instances of the same npm-based formatter to be used by separating their `node_modules` directories. ([#​1565](diffplug/spotless#1565)) - `ktfmt` default style uses correct continuation indent. ([#​1562](diffplug/spotless#1562)) ##### Changes - Bump default `ktfmt` version to latest `0.42` -> `0.43` ([#​1561](diffplug/spotless#1561)) - Bump default `jackson` version to latest `2.14.1` -> `2.14.2` ([#​1536](diffplug/spotless#1536)) </details> <details> <summary>quarkusio/quarkus</summary> ### [`v2.16.6.Final`](https://github.com/quarkusio/quarkus/releases/tag/2.16.6.Final) [Compare Source](quarkusio/quarkus@2.16.5.Final...2.16.6.Final) ##### Complete changelog - [#​32319](quarkusio/quarkus#32319) - \[2.16] Revert io.netty.noUnsafe change - [#​32302](quarkusio/quarkus#32302) - Qute - fix validation of expressions with the "cdi" namespace - [#​32253](quarkusio/quarkus#32253) - (2.16) Upgrade to graphql-java 19.4 - [#​32223](quarkusio/quarkus#32223) - (2.16) Upgrade wildfly-elytron to 1.20.3.Final - [#​32110](quarkusio/quarkus#32110) - Prevent splitting of cookie header values when using AWS Lambda - [#​32107](quarkusio/quarkus#32107) - Fix Podman detection on Windows - [#​32106](quarkusio/quarkus#32106) - Native building with container: Podman not detected on Windows - [#​32093](quarkusio/quarkus#32093) - Re-use current ApplicationModel for JaCoCo reports when testing Gradle projects - [#​32090](quarkusio/quarkus#32090) - K8s moved its registry - [#​32088](quarkusio/quarkus#32088) - Remove the session cookie if ID token verification failed - [#​32082](quarkusio/quarkus#32082) - Add missing quote in Hibernate Reactive with Panache guide - [#​32079](quarkusio/quarkus#32079) - Quarkus JaCoCo extension fails to start Gradle daemon - [#​32058](quarkusio/quarkus#32058) - Allow use of null in REST Client request body - [#​32047](quarkusio/quarkus#32047) - rest client reactive throws npe on null request body - [#​32041](quarkusio/quarkus#32041) - K8s is moving it's images - [#​32037](quarkusio/quarkus#32037) - Set-Cookie Header is Split when using OIDC together with AWS Lambda - [#​32015](quarkusio/quarkus#32015) - Support repeatable Incomings annotation for reactive messaging - [#​32002](quarkusio/quarkus#32002) - Quarkus: Kafka Event Processor with 2 `@incoming` annotations throws Null Pointer SRMSG00212 - [#​31984](quarkusio/quarkus#31984) - Only substitute OctetKeyPair\* classes when on the classpath - [#​31978](quarkusio/quarkus#31978) - Remove quarkus.hibernate-orm.database.generation=drop-and-create from Hibernate ORM codestart - [#​31930](quarkusio/quarkus#31930) - Native build fails for JWT - [#​31893](quarkusio/quarkus#31893) - Docker or Podman required for tests since 3.0.0.Alpha6 - [#​31857](quarkusio/quarkus#31857) - Container runtime detection cached in sys prop, container-docker extension - [#​31811](quarkusio/quarkus#31811) - Check the expiry date for inactive OIDC tokens - [#​31717](quarkusio/quarkus#31717) - Quarkus OIDC Session Cookie not deleted in case of 401 unauthorized - [#​31714](quarkusio/quarkus#31714) - OIDC token refresh fails with 401, if user info is used and not available in the cache (anymore) - [#​31662](quarkusio/quarkus#31662) - Warning when docker is not running - [#​31525](quarkusio/quarkus#31525) - Bump Keycloak version to 21.0.1 - [#​31490](quarkusio/quarkus#31490) - Enable Podman and Docker Windows quarkus-container-image-docker testing - [#​31307](quarkusio/quarkus#31307) - Native Build on Windows has incorrect resource slashes - [#​30383](quarkusio/quarkus#30383) - Create a new base classloader including parent-first test scoped dependencies when bootstrapping for CT ### [`v2.16.5.Final`](https://github.com/quarkusio/quarkus/releases/tag/2.16.5.Final) [Compare Source](quarkusio/quarkus@2.16.4.Final...2.16.5.Final) ##### Complete changelog - [#​31959](quarkusio/quarkus#31959) - New home for Narayana LRA coordinator Docker images - [#​31931](quarkusio/quarkus#31931) - Support raw collections in RESTEasy Reactive server and client - [#​31922](quarkusio/quarkus#31922) - Add more lenient Liquibase ZipPathHandler to work around includeAll not working in prod mode - [#​31904](quarkusio/quarkus#31904) - \[2.16] Upgrade SmallRye GraphQL to 1.9.4 - [#​31894](quarkusio/quarkus#31894) - Supply missing extension metadata for reactive keycloak client - [#​31891](quarkusio/quarkus#31891) - Fix truststore REST Client config when password is not set - [#​31867](quarkusio/quarkus#31867) - Qute type-safe fragments - fix validation for loop metadata and globals - [#​31866](quarkusio/quarkus#31866) - The behavior of the `@RestHeader` annotation is different from the `@HeaderParam` annotation when the parameter is of type List - [#​31864](quarkusio/quarkus#31864) - Fix incorrect generic type passed to MessageBodyWriter#writeTo - [#​31818](quarkusio/quarkus#31818) - Jackson JAX-RS YAML Provider for Resteasy Reactive - [#​31804](quarkusio/quarkus#31804) - \[2.16] A test to make sure non-existing modules are ignored during workspace discovery - [#​31793](quarkusio/quarkus#31793) - \[2.16] Fix NPE loading workspace modules - [#​31770](quarkusio/quarkus#31770) - Fix native compilation when using quarkus-jdbc-oracle with elasticsearch-java - [#​31769](quarkusio/quarkus#31769) - Capability added for quarkus-rest-client-reactive-jackson - [#​31756](quarkusio/quarkus#31756) - quarkus-rest-client-reactive-jackson doesn't provide capabilities - [#​31728](quarkusio/quarkus#31728) - Register additional cache implementations for reflection - [#​31718](quarkusio/quarkus#31718) - Properly close metadata file in integration tests - [#​31713](quarkusio/quarkus#31713) - "Too many open files" When test native image. - [#​31712](quarkusio/quarkus#31712) - Make request scoped beans work properly in ReaderInterceptors - [#​31705](quarkusio/quarkus#31705) - Remove all dev services for kubernetes dependencies from kubernetes-client-internal - [#​31692](quarkusio/quarkus#31692) - RequestScoped context not active when using a ReaderInterceptor with large HTTP requests - [#​31688](quarkusio/quarkus#31688) - Suppress config changed warning for quarkus.test.arg-line - [#​31643](quarkusio/quarkus#31643) - Fix iterator issue when executing a zrange with score on a missing key - [#​31626](quarkusio/quarkus#31626) - quarkus.test.arg-line has become a built-time fixed property in 2.16.4 - [#​31624](quarkusio/quarkus#31624) - native compilation : quarkus-jdbc-oracle with elasticsearch-java strange behaviour - [#​31617](quarkusio/quarkus#31617) - Bump Stork version 1.4.2 - [#​31579](quarkusio/quarkus#31579) - Reinitialize sun.security.pkcs11.P11Util at runtime - [#​31560](quarkusio/quarkus#31560) - Prevent SSE writing from potentially causing accumulation of headers - [#​31559](quarkusio/quarkus#31559) - `SseUtil` unexpectedly stores headers in `Serialisers.EMPTY_MULTI_MAP` - [#​31551](quarkusio/quarkus#31551) - Scheduler - detect scheduled methods of the same name on a class - [#​31547](quarkusio/quarkus#31547) - Scheduler - it's possible to declare two scheduled methods of the same name on the same class - [#​31545](quarkusio/quarkus#31545) - Append System.lineSeparator() to config error messages - [#​31536](quarkusio/quarkus#31536) - Missing newline characters in config error message - [#​31532](quarkusio/quarkus#31532) - Interpret negative/zero body-limit as infinite when logging REST Client request body - [#​31523](quarkusio/quarkus#31523) - Request rejected by CORS for fonts in dev UI when `quarkus.http.cors=true` is set - [#​31496](quarkusio/quarkus#31496) - Filter out RESTEasy related warning in ProviderConfigInjectionWarningsTest - [#​31482](quarkusio/quarkus#31482) - Remove incorrect default value for keepAliveEnabled - [#​31440](quarkusio/quarkus#31440) - Several quarkus integration tests fail to compile to native with latest GraalVM master - [#​31384](quarkusio/quarkus#31384) - Ignore required documentation for `@ConfigMapping` default methods - [#​30757](quarkusio/quarkus#30757) - Allow same origin CORS requests without 3rd party origins being configured - [#​30744](quarkusio/quarkus#30744) - \[Quarkus Native] ClassNotFoundException: com.github.benmanes.caffeine.cache.SSSW - [#​30698](quarkusio/quarkus#30698) - CORS Request same origin ignored if no other origin set </details> <details> <summary>quarkusio/quarkus-platform</summary> ### [`v2.16.6.Final`](quarkusio/quarkus-platform@2.16.5.Final...2.16.6.Final) [Compare Source](quarkusio/quarkus-platform@2.16.5.Final...2.16.6.Final) ### [`v2.16.5.Final`](quarkusio/quarkus-platform@2.16.4.Final...2.16.5.Final) [Compare Source](quarkusio/quarkus-platform@2.16.4.Final...2.16.5.Final) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This MR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4yNC4wIiwidXBkYXRlZEluVmVyIjoiMzQuMjQuMCJ9-->
This version introduces Loom-friendly synchronization (pgjdbc/pgjdbc#2635)
This version introduces Loom-friendly synchronization (pgjdbc/pgjdbc#2635) (cherry picked from commit c2bf5aa)
This version introduces Loom-friendly synchronization (pgjdbc/pgjdbc#2635)
Hi, there is still some synchronized occurences (in core/v3/QueryExecutorImpl.java and util/LazyCleaner.java). Is it safe to use pgjdbc in multithread execution with Java 21 without potential performance issues ? |
Java 21 supports |
Yes but what if we start using Virtual Threads (cf. https://blog.fastthread.io/2023/02/28/pitfalls-to-avoid-when-switching-to-virtual-threads/ for example) ? |
The use of synchronised in QueryExecutorImpl is not an issue for Virtual Threads. Looking at: https://github.com/pgjdbc/pgjdbc/blob/master/pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java#L2967-L3040 ... we note there is that there is nothing executed INSIDE the synchronized blocks (like IO etc) that would park a Virtual Thread. That is, the code executed inside the synchronised blocks are just manipulations of the local useBinaryReceiveForOids HashSet to no Virtual Thread parking will incur in there and so this is all fine. LazyCleaner to me is similar apart from the method https://github.com/pgjdbc/pgjdbc/blob/master/pgjdbc/src/main/java/org/postgresql/util/LazyCleaner.java#L105-L169 which includes Note: For myself, I do have applications using Virtual Threads (via Helidon 4.x so all requests are using Virtual Threads for these apps) and these apps have all been working fine - I've observed no issues with thread pinning etc. Not sure if that is because Q: Can we confirm if |
Are there any issues with changing this to ReentrantLock ? I have no objections. Ideally Java 8 goes away (haha) and we get rid of this code :) |
I think it should not park. It is just creating a new thread, see pgjdbc/pgjdbc/src/main/java/org/postgresql/util/LazyCleaner.java Lines 71 to 75 in 24f2c7e
I'm +-0 on that. Of course, as we already use |
Referencing #1951 for outline / motivation which is to improve compatibility with loom.
Change PgStatement, PgPreparedStatement and PgCallableStatement replacing synchronized (this) blocks with use of ReentrantLock to be compatible with loom.
In doing this change for PgStatement this PR must include:
All synchronized (this) blocks in PgStatement
All synchronized (this) blocks in it's sub types (PgPreparedStatement and PgCallableStatement)
I confirm that this PR includes all synchronized (this) blocks across PgStatement and all it's sub types.
Note that this PR does not add any new tests or change any existing tests. I do not believe either is required.