refactor:(loom) replace the usages of synchronized with ReentrantLock

[rbygrave]

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.

[vlsi]

The PR looks good to me, except there's still some synchronized (connection) { left in PgStatement.java: https://github.com/rbygrave/pgjdbc/blob/feature/1951-PgStatement-3/pgjdbc/src/main/java/org/postgresql/jdbc/PgStatement.java#L977-L984

Have you tried adjusting it as well?

I'm inclined we might get concurrency issues if we replace only a fraction of synchronizations on Statement with Lock.

[rbygrave]

synchronized (connection)

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.

• With the lock on connection we use .notifyAll() and .wait(). This is no problem as those are supported by ReentrantLock.
• We need to decide how to expose the PgConnection lock to PgStatement. One option is change the PgStatement.connection type to PgConnection and then use a package private method on PgConnection. A second option is to add a method to the BaseConnection interface (which would be a public method so for myself I'd be less keen for this option).

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).

get concurrency issues if we replace only a fraction of synchronizations

We need to specifically be careful wrt use of synchronized (somethingThatIsNotThis) ... like for example synchronized (connection). For example, in the "lock on connection" case we easily find all the uses inside of PgConnection (and subclasses if it had any) but we also search globally for code like synchronized (connection) and we need to be sure we find all the cases and this part is a manual search we need to do carefully.

In this case of the lock on PgStatement [and subclasses] we need to:

• Change all synchronized methods in PgStatement and subclasses
• Change all synchronized (this) in PgStatement and subclasses
• Find all uses of the lock on PgStatement instances in the rest of the code by globally searching synchronized ( ... checking there are no cases of synchronized (statement) or similar [edit: and in this case there are no other uses of the lock]

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 synchronized (connection) there unmodified.

[vlsi]

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 increase and decrease methods separately because if we merge a PR that changes only increase from synchronized to ReentrantLock, then we basically get a broken class like:

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 increase and decrease concurrently would break.

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 pgjdbc unreleasable until we merge all the changes.

[rbygrave]

Are we ok changing the type of PgStatement.connection to PgConnection from BaseConnection?

See my comment from "We need to decide how to expose the PgConnection lock to PgStatement."

[vlsi]

By the way, @rbygrave , have you considered adding something like AutoCloseableLock extends ReentrantLock for patterns like try (ResourceLock ignored = obtainLock()) { ... }?

[rbygrave]

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 ...
A) a wrapper type (downside of extra instance per lock)
B) a type that extends ReentrantLock (downside, can be used incorrectly in try-with-resources)

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.

[vlsi]

B) a type that extends ReentrantLock (downside, can be used incorrectly in try-with-resources)

Could you please clarify what you mean by "can be used incorrectly"?
I think a pattern like "use locks in try-with-resources only" should be less prone to errors than "use raw Lock API" (e.g. one might forget to add finally).

Happy to try an AutoClosableLock if you or others are keen

I think it is a small addition that makes the code slightly easier to read/write, and it brings no downsides performance-wise.
We could have used withLock(() -> ...) pattern, however, it would incur overhead.

[rbygrave]

can be used incorrectly

With B) try (autoClosableLock) { will compile but be invalid - this is the way in which (B) can be used incorrectly. We always need to use a method like obtainLock() such that .lock() is called like: try (ResourceLock ignored = autoClosableLock.obtainLock()).

be less prone to errors than "use raw Lock API" (e.g. one might forget to add finally).

It's a compile error to add the try { ... } without a finally [or catch] so for myself I don't see that as less error prone. For example, when we replace the synchronized (this) { with the lock.lock(); try { ... we are always in effect adding a new try block so it will not compile without adding the finally. The only way to get it wrong is to add a catch () { } block instead of a finally { ... }. IntelliJ will have a little red marker showing us where the missing finally block needs to go.

Said another way, for this change we can never re-use an existing try { block - we always need to add a new try { ... so in that way the compiler tells us to add a finally { ... } [or catch (..) { .. }].

We could have used withLock(() -> ...)

Yes agreed, closure overhead there.

[rbygrave]

Did that make sense @vlsi ? Are you still keen for us to create a ResourceLock extends ReentrantLock as per (B) and use it via try (ResourceLock ignored = lock.obtainLock()) { ... } and remove the finally blocks or keep it as it current is with the finally blocks?

[davecramer]

@rbygrave seems @vlsi may be out of touch for a bit. I'd like to see this move forward. For now use what you think is right and if Vladimir strenuously objects later we can change it.

[vlsi]

It's a compile error to add the try { ... } without a finally [or catch] so for myself I

The following would be a misuse of the lock API which would be impossible with try-with-resources.

lock.lock()
try {
    ...
} catch(SQLException e) {
    e.printStacktrace(); // or whatever
}

We always need to use a method like obtainLock() such that .lock() is called like: try (ResourceLock ignored = autoClosableLock.obtainLock()).

Well, there could be a private (or package-private) method obtainLock, so the usage would be as follows:

try (AutoClosable ignored = obtainLock()) {
    ...
}

[rbygrave]

The following would be a misuse of the lock API

Yes agreed. That is what I said as well.

so the usage would be as follows:

Pretty much but I would suggest that obtainLock() would instead be lock.obtainLock() because we would not want to add a local helper method on each and every type that we use a "ResourceLock" so I'd suggest it would end up as:

try (AutoClosable ignored = lock.obtainLock()) {
    ...
}

For myself, my gut says that I'd prefer being more explicit using ResourceLock rather than AutoClosable like:

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 ResourceLock extends ReentrantLock implements AutoClosable as per (B) and use it via try (AutoClosable ignored = lock.obtainLock()) { ... } and remove the finally blocks or keep it as it currently is with the finally blocks?

For me I'm happy either way. I could even make that change using a ResourceLock and push another commit and then everyone gets to see both styles explicitly. If we don't like it we could just remove that last commit. Should we do that?

[rbygrave]

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)?
Do you want me to push another commit with that approach or keen to go with what we have with the finally blocks?

Cheers, Rob.

[vlsi]

I incline to ResourceLock extends ReentrantLock implements AutoClosable.

[davecramer]

I incline to ResourceLock extends ReentrantLock implements AutoClosable.

I'll defer to @vlsi as I am not an expert in this area

[rbygrave]

Ok, added ResourceLock and using that with try-with-resources. Let me know what you think.

[rbygrave]

Had a chance to look at it yet?

[vlsi]

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.
Do you think you could figure out the reason for the test failures?

[davecramer]

I'm surprised our code style check doesn't pick up on the lack of the header in ResourceLock and ResourceLockTest

[vlsi]

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.

[Powerrr]

Do you think you could figure out the reason for the test failures?

I believe this happens because of incorrect Lock API usage: we shoudn't use .wait()/.notify() on locks - these methods are inherited from Object and are using the same object monitor as synchronized blocks.
It seems that the correct way is to use the Condition object that can be obtained using lock.newCondition(), something like

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 Condition per lock (the substitute for the object monitor), we can add it to the ResourceLock helper:

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();
  }

  //...
}

[gavlyukovskiy]

Hi, just a heads up - I've tried running some loom tests on this PR with -Djdk.tracePinnedThreads=full and virtual threads still pin on synchronized methods inside QueryExecutorImpl:

    org.postgresql.core.v3.QueryExecutorImpl.execute(QueryExecutorImpl.java:356) <== monitors:1
    org.postgresql.jdbc.PgStatement.executeInternal(PgStatement.java:497)
    org.postgresql.jdbc.PgStatement.execute(PgStatement.java:414)
    org.postgresql.jdbc.PgPreparedStatement.executeWithFlags(PgPreparedStatement.java:190)
    org.postgresql.jdbc.PgPreparedStatement.executeQuery(PgPreparedStatement.java:134)
    com.zaxxer.hikari.pool.ProxyPreparedStatement.executeQuery(ProxyPreparedStatement.java:52)

Full stacktrace:

Thread[#67,ForkJoinPool-1-worker-1,5,CarrierThreads]
    java.base/java.lang.VirtualThread$VThreadContinuation.onPinned(VirtualThread.java:180)
    java.base/jdk.internal.vm.Continuation.onPinned0(Continuation.java:398)
    java.base/jdk.internal.vm.Continuation.yield0(Continuation.java:390)
    java.base/jdk.internal.vm.Continuation.yield(Continuation.java:357)
    java.base/java.lang.VirtualThread.yieldContinuation(VirtualThread.java:370)
    java.base/java.lang.VirtualThread.park(VirtualThread.java:499)
    java.base/java.lang.System$2.parkVirtualThread(System.java:2596)
    java.base/jdk.internal.misc.VirtualThreads.park(VirtualThreads.java:54)
    java.base/java.util.concurrent.locks.LockSupport.park(LockSupport.java:369)
    java.base/sun.nio.ch.Poller.poll2(Poller.java:139)
    java.base/sun.nio.ch.Poller.poll(Poller.java:102)
    java.base/sun.nio.ch.Poller.poll(Poller.java:87)
    java.base/sun.nio.ch.NioSocketImpl.park(NioSocketImpl.java:175)
    java.base/sun.nio.ch.NioSocketImpl.park(NioSocketImpl.java:196)
    java.base/sun.nio.ch.NioSocketImpl.implRead(NioSocketImpl.java:304)
    java.base/sun.nio.ch.NioSocketImpl.read(NioSocketImpl.java:340)
    java.base/sun.nio.ch.NioSocketImpl$1.read(NioSocketImpl.java:789)
    java.base/java.net.Socket$SocketInputStream.read(Socket.java:1025)
    org.postgresql.core.VisibleBufferedInputStream.readMore(VisibleBufferedInputStream.java:161)
    org.postgresql.core.VisibleBufferedInputStream.ensureBytes(VisibleBufferedInputStream.java:128)
    org.postgresql.core.VisibleBufferedInputStream.ensureBytes(VisibleBufferedInputStream.java:113)
    org.postgresql.core.VisibleBufferedInputStream.read(VisibleBufferedInputStream.java:73)
    org.postgresql.core.PGStream.receiveChar(PGStream.java:453)
    org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:2120)
    org.postgresql.core.v3.QueryExecutorImpl.execute(QueryExecutorImpl.java:356) <== monitors:1
    org.postgresql.jdbc.PgStatement.executeInternal(PgStatement.java:497)
    org.postgresql.jdbc.PgStatement.execute(PgStatement.java:414)
    org.postgresql.jdbc.PgPreparedStatement.executeWithFlags(PgPreparedStatement.java:190)
    org.postgresql.jdbc.PgPreparedStatement.executeQuery(PgPreparedStatement.java:134)
    com.zaxxer.hikari.pool.ProxyPreparedStatement.executeQuery(ProxyPreparedStatement.java:52)
    com.zaxxer.hikari.pool.HikariProxyPreparedStatement.executeQuery(HikariProxyPreparedStatement.java)
    com.github.gavlyukovskiy.app.PostgresRepository.getWorld(SpringWebApplication.kt:85)
    com.github.gavlyukovskiy.app.Controller$db$1.invoke(SpringWebApplication.kt:144)
    com.github.gavlyukovskiy.app.Controller$db$1.invoke(SpringWebApplication.kt:144)
    com.github.gavlyukovskiy.app.Controller.record(SpringWebApplication.kt:152)
    com.github.gavlyukovskiy.app.Controller.db(SpringWebApplication.kt:144)
    java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
    java.base/java.lang.reflect.Method.invoke(Method.java:578)
    org.springframework.web.method.support.InvocableHandlerMethod.doInvoke(InvocableHandlerMethod.java:207)
    org.springframework.web.method.support.InvocableHandlerMethod.invokeForRequest(InvocableHandlerMethod.java:152)
    org.springframework.web.servlet.mvc.method.annotation.ServletInvocableHandlerMethod.invokeAndHandle(ServletInvocableHandlerMethod.java:117)
    org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.invokeHandlerMethod(RequestMappingHandlerAdapter.java:884)
    org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.handleInternal(RequestMappingHandlerAdapter.java:797)
    org.springframework.web.servlet.mvc.method.AbstractHandlerMethodAdapter.handle(AbstractHandlerMethodAdapter.java:87)
    org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:1080)
    org.springframework.web.servlet.DispatcherServlet.doService(DispatcherServlet.java:973)
    org.springframework.web.servlet.FrameworkServlet.processRequest(FrameworkServlet.java:1003)
    org.springframework.web.servlet.FrameworkServlet.doGet(FrameworkServlet.java:895)
    jakarta.servlet.http.HttpServlet.service(HttpServlet.java:500)
    org.springframework.web.servlet.FrameworkServlet.service(FrameworkServlet.java:880)
    jakarta.servlet.http.HttpServlet.service(HttpServlet.java:587)
    org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:223)
    org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:158)
    org.springframework.web.filter.RequestContextFilter.doFilterInternal(RequestContextFilter.java:100)
    org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116)
    org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:185)
    org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:158)
    org.springframework.web.filter.FormContentFilter.doFilterInternal(FormContentFilter.java:93)
    org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116)
    org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:185)
    org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:158)
    org.springframework.web.filter.ServerHttpObservationFilter.doFilterInternal(ServerHttpObservationFilter.java:109)
    org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116)
    org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:185)
    org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:158)
    org.springframework.web.filter.CharacterEncodingFilter.doFilterInternal(CharacterEncodingFilter.java:201)
    org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116)
    org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:185)
    org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:158)
    org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:197)
    org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:97)
    org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:542)
    org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:119)
    org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:92)
    org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:78)
    org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:357)
    org.apache.coyote.http11.Http11Processor.service(Http11Processor.java:400)
    org.apache.coyote.AbstractProcessorLight.process(AbstractProcessorLight.java:65)
    org.apache.coyote.AbstractProtocol$ConnectionHandler.process(AbstractProtocol.java:861)
    org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1739)
    org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:52)
    java.base/java.util.concurrent.ThreadPerTaskExecutor$TaskRunner.run(ThreadPerTaskExecutor.java:314)
    java.base/java.lang.VirtualThread.run(VirtualThread.java:287)
    java.base/java.lang.VirtualThread$VThreadContinuation.lambda$new$0(VirtualThread.java:174)
    java.base/jdk.internal.vm.Continuation.enter0(Continuation.java:327)
    java.base/jdk.internal.vm.Continuation.enter(Continuation.java:320)

[rbygrave]

Hi, just a heads up - ... and virtual threads still pin on synchronized methods inside QueryExecutorImpl

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 CopyOperationImpl.isActive() where I have added a helper method and people reading the diff might need to look twice etc (I'll add a comment into the PR to point this bit out).

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.

[rbygrave]

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)

[apatrida]

@vlsi nor does Heinz share the benchmark, odd.

[jerometambo]

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 ?

[vlsi]

Java 21 supports synchronized, so go ahead and use pgjdbc with Java 21

[jerometambo]

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) ?

[rob-bygrave]

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 Thread thread = threadFactory.newThread(new Runnable() { .... The question here is, could a Virtual Thread park with the threadFactory.newThread() call? For myself I'm not sure.

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 threadFactory.newThread() doesn't park VT or more if I've been lucky somehow.

Q: Can we confirm if threadFactory.newThread() can park a virtual thread?
Q: Do we want to change LazyCleaner over to ReentrantLock just to be safer?

[davecramer]

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 Thread thread = threadFactory.newThread(new Runnable() { .... The question here is, could a Virtual Thread park with the threadFactory.newThread() call? For myself I'm not sure.

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 threadFactory.newThread() doesn't park VT or more if I've been lucky somehow.

Q: Can we confirm if threadFactory.newThread() can park a virtual thread? Q: Do we want to change LazyCleaner over to ReentrantLock just to be safer?

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 :)

[vlsi]

Q: Can we confirm if threadFactory.newThread() can park a virtual thread?

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

	this(threadTtl, runnable -> {
	Thread thread = new Thread(runnable, threadName);
	thread.setDaemon(true);
	return thread;
	});

Q: Do we want to change LazyCleaner over to ReentrantLock just to be safer?

I'm +-0 on that. Of course, as we already use ReentrantLock, it should not be much worse if we refactor LazyCleaner. It would deviate from the original source though.