Skip to content

Fixes for virtual threads - #526

Open
lachlan-roberts wants to merge 1 commit into
mainfrom
virtualthreads-jdk21-fixes
Open

lachlan-roberts wants to merge 1 commit into
mainfrom
virtualthreads-jdk21-fixes

Conversation

@lachlan-roberts

Copy link
Copy Markdown
Collaborator

Problem

With appengine.use.virtualthreads=true on java21 in HTTP connector mode, AppLogsWriter waits for the previous log-flush RPC inside synchronized. On JDK 21 that pins the carrier thread, and an F1 instance reports one CPU so it has only one carrier — every other request on the instance stalls until the flush returns.

RPC mode (still in released versions; removed from main in 8f12d12f) is unaffected because it never runs application code on a virtual thread. java25 is unaffected because JDK 24+ no longer pins in synchronized (JEP 491).

This is what users hit on v5.0.x. On v5.1.0 and current main the Jetty 12 adapter does not run requests on virtual threads at all (63422a21 replaced the executor with a bounded ForkJoinPool — see Notes), so today the pin is reachable only through the Jetty 12.1 adapter (appengine.use.jetty121=true) or virtual threads the application creates itself. It comes back for the default java21 path as soon as #523 restores the real executor, so this fix should land with or before it.

Changes

  • AppLogsWriter (runtime and api/setup): synchronized → ReentrantLock, restoring the code from before 63422a21. A virtual thread unmounts while waiting on a ReentrantLock, so the pin is gone; this is the fix JEP 444 recommends. The legacy/virtual-thread fork added in 63422a21 is dropped: it existed only to work around the monitor, and it had bugs (split log messages could interleave with a child thread's lines; flushAndWait() could return without flushing the caller's lines if its wait on the previous flush was interrupted or timed out).
  • Remove the two tests that assert the fork's behaviour. The other three tests added in 63422a21 still pass and are kept.
  • Remove JavaRuntimeMain.configureVirtualThreadParallelism(). availableProcessors() already reflects the instance's CPU quota (measured: 1 on an F1, 2 on a B8), so the cap was a no-op on small instances and raised parallelism to 4 on 2 GB ones.
  • Remove the unused getMaxSafeCarrierParallelism() from the Jetty 12.1 adapter, its test, and e2etest.md, an internal planning note that references it and misdescribes how RPC mode dispatches.

Notes

@lachlan-roberts lachlan-roberts self-assigned this Sep 30, 2026
Use a ReentrantLock instead of synchronized so waiting on a log flush no
longer pins the carrier thread on JDK 21, restoring the pre-63422a21
structure of both writers. Remove the GAE_MEMORY_MB parallelism cap; the
JVM's CPU-quota default already covers it.

Signed-off-by: Lachlan Roberts <lachlan.p.roberts@gmail.com>
@ludoch

ludoch commented Oct 2, 2026

Copy link
Copy Markdown
Collaborator

Note: A combined PR with the latest review updates and fixes from both #523 and #526 is available in #528 (#528).

@ludoch

ludoch commented Oct 2, 2026

Copy link
Copy Markdown
Collaborator

Do another review. It cannot be merge in gh as there are also non visible changes in google3 needed for this change, so do your own review first here.

@ludoch

ludoch commented Oct 2, 2026

Copy link
Copy Markdown
Collaborator

@lachlan-roberts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants