Server-side backend: a native, JVM-free runtime for Codename One handlers - #5741
Open
shai-almog wants to merge 190 commits into
Open
Server-side backend: a native, JVM-free runtime for Codename One handlers#5741shai-almog wants to merge 190 commits into
shai-almog wants to merge 190 commits into
Conversation
The clean (non-Objective-C) target could translate a Java main() and run it, but
not much more: main(String[]) was handed JAVA_NULL, so a translated program could
not read its own command line, and there was no way to read the environment, open
a file or read stdin. Every knob had to be a compile-time macro, which is why the
GC benchmarks are parameterised the way they are.
- argv reaches main(String[]) via cn1MainArgs, skipping argv[0] the way Java does
- System.getenv(String)
- java.io.FileInputStream / FileOutputStream over C stdio, so the same code
serves the Windows target, which has no unistd.h
- java.io.StandardInputStream behind System.in. Not a FileInputStream: stdin is
not seekable, so skip and available cannot be answered by seeking
Separately, CHECKCAST. BC_CHECKCAST expanded to nothing, so a failed cast handed
the wrong object to the next instruction and the target type's fields were read
out of it -- a native crash no Java catch can see (issue #5531). Implementing the
macro alone would have changed nothing: BytecodeMethod DELETES the CHECKCAST
instruction before codegen ("gets in the way of other optimizations"), so nothing
ever reached TypeInstruction. Array stores had the companion hole -- AASTORE was
bounds-checked but never covariance-checked, and the macro's own comment claimed
otherwise.
Both are now enforced under -Dcn1.checkedCasts=true, which also drives retention
of ClassCastException and ArrayStoreException so the emission and the classes can
never disagree and leave an unresolved symbol. Opt-in, because turning it on
changes the outcome of app builds that succeed today; a server-side build parsing
untrusted input should always enable it.
Verified against vm/tests: 80 tests, no regressions.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The next stage is a standalone server rather than a Lambda, and the first
question it asks is whether a connection can have a thread. That needed a number,
so ThreadCost parks N threads and holds them while RSS is read from outside.
Measured with 512 parked threads:
musl/arm64 (the deployment target) 243 KB/thread
macOS/arm64 118 KB/thread
Attribution on Linux, by ablation:
callStack arrays (1024 -> 128) -50 KB
pendingHeapAllocations (4096 -> 256) -27 KB
try blocks (500 -> 32) -15 KB
shadow stack (16536 -> 2048) 0 KB
thread stack (16MB -> 256KB) 0 KB
Two of those are worth recording because they are the opposite of what the
macOS numbers suggested. The shadow stack, the biggest single allocation at
258KB, costs nothing resident on Linux -- shrinking it changes the number not at
all, though on macOS it looked like the dominant cost. And the pinned 16MB thread
stack is free: it is reserved, never committed.
The five sizes are now #ifndef-guarded so an A/B can override them with -D. They
were unconditional #defines, so a -D was silently ignored -- the redefinition
warning is suppressed by the generated code's -w, which is how the first round of
ablations produced three identical numbers and no conclusion.
The shadow stack is now mapped rather than malloc'd and memset in full. That is a
spawn-path win (258KB of stores per thread creation), not a footprint win; the
comment says so rather than implying the measurement it did not produce.
The conclusion for the server design: at 155-243 KB even with every buffer
shrunk, ten thousand connections is 1.5-2.4GB of threads. A connection cannot have
one. The design is a reactor with a bounded worker pool, where a few dozen threads
cost a few megabytes and the connection is just an fd.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
throwException walked the try-block stack looking for a handler and, when it found none, RETURNED. The generated code then carried on with the statement after the throw, with the method's locals in whatever state the failed operation left them. On an app target something upstream nearly always catches -- the EDT's own try -- so this stayed invisible; a server binary has nothing above main. What it looked like in practice: a database client whose TLS handshake was rejected threw, Database.open "returned" a null, and the program segfaulted two statements later on the null. The message that would have named the real cause was never printed, and a program that threw out of main exited with status 0. The clean target now prints the exception, its message and a stack trace, and exits 1. Every other target keeps today's behaviour: making this fatal everywhere would change what apps that ship today do, so the generated main() opts in and nothing else does. Two details the fix needed. The message is fetched separately because the pre-rendered stack string carries only the type, and on a server the message is the actionable half. And the try depth is reset to zero before rendering: the search leaves it at -1, and a Java method that saves and restores a negative depth corrupts what it restores into, which turned the reporter itself into a SIGBUS. Also here, because the same audit found it: java.lang.System.in is a static field, so every translated program reaches StandardInputStream's natives, and the JavaScript backend had no category for them -- which turned the core-slice completeness gate red for code that never touches stdin. They are marked unsupported there, as java.io.File already is: a browser has no process stdin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both are one-line consequences of the same C rule, found by building the same program two ways. ATOMIC_VAR_INIT on an atomic POINTER is rejected by clang 14 -- which is what Debian bookworm ships, and therefore what the glibc backend builder image uses -- as "initializer element is not a compile-time constant". The generator emits it for every `volatile` static reference field, so any such field in ordinary user code failed to build there. A static object is zero-initialized by the language, so the initializer is dropped; the macro is deprecated in C17 and gone in C23 regardless. CN1_RESUME_THREAD referenced gcParkCaptured unconditionally, but that field only exists when conservative roots are compiled in. So -DCN1_DISABLE_CONSERVATIVE_GC_ROOTS -- the A/B arm vm/CLAUDE.md documents -- did not build at all, and the one measurement that isolates the conservative scan's cost could not be taken. It is now behind a macro that compiles away with the field. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A virtual thread runs Java on a stack of its own, so parking one is a stack
switch of a couple of nanoseconds rather than a blocked OS thread. Measured
round trip on arm64: 2.1ns.
The runtime is three files -- cn1_virtual_thread.{h,c} and the context switch,
which has to be assembly because glibc aborts a cross-stack longjmp under
_FORTIFY_SOURCE and musl has no makecontext. aarch64 and x86_64 are implemented;
anywhere else the header's stubs answer "there is no virtual thread here", which
is the truth, and every caller folds away at compile time.
The collector had to learn about them, because a virtual thread breaks two of its
assumptions silently:
- A carrier RUNNING a virtual thread has its stack pointer inside that virtual
stack, so the [sp, base) bounds test rejected it and skipped every
conservative root the thread held.
- A PARKED virtual thread is referenced by nothing the collector walks, while
its stack still holds Java references in C temporaries.
Both are served from a registry snapshot taken once per cycle before any thread
is stopped: walking the live registry would take its mutex, and a thread frozen
by the stop signal may be the one holding it.
Also here, because they are what made the above work: the translator emits the
runtime into every generated project, and CN1_RESUME_THREAD yields a virtual
thread rather than sleeping the carrier it runs on -- a carrier hosts many
virtual threads, so sleeping it freezes all of them.
Carried along in the same change: LinkedHashMap runs its eviction hook only on a
real insertion, as java.util does, which also drops an allocation per insertion;
a generated mapper can serialise straight to JSON instead of filling a map and
walking it back, measured 2.05x/1.51x/2.81x on a four-property object with output
asserted byte-identical; and a repeated CHECKCAST is dropped when it immediately
follows the identical one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CN1_RESUME_THREAD waited out a collection with usleep(1000). Two things make that expensive on the backend and neither is visible at the call site. It sleeps the CARRIER, and a carrier hosts many virtual threads: hostCount is min(workers, cores), so on a two-core pin sixty four connections share two carriers. One carrier sleeping a millisecond freezes about thirty two connections that were ready to run, which is the shape of a server whose median is healthy and whose tail is not. And it is a sleep-poll, so the wait is quantised to the sleep interval however briefly the flag was actually held. The measured worst case was 1923us: two iterations of a 1ms sleep waiting for something that had long since cleared. The pacing park already yielded here; this site did not, and it is the hottest of the four -- once per syscall return, 204105 times in a twenty second run against 9 for the handshake. Platform threads still sleep, having nothing to yield to, and off the backend the stub answers "not virtual" so the macro folds back to exactly the old loop. This shortens the wait; it does not remove it. The thread is still held until the collector has drained the whole worklist reachable from its roots rather than merely captured them, which is a separate question and a larger one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
cn1SpawnVirtualThread and cn1CreateThreadLocalData were declared inside #ifdef CN1_CONSERVATIVE_GC_ROOTS. Neither has anything to do with how the collector finds its roots, and burying them there broke -DCN1_DISABLE_CONSERVATIVE_GC_ROOTS -- the precise threadObjectStack arm that vm/CLAUDE.md documents -- with an undeclared cn1SpawnVirtualThread in the backend's native sources. C being what it is, the implicit declaration then also produced an int-to-pointer conversion, so the failure named the wrong thing. Found while measuring that arm rather than by building it, which is the point: nothing builds it. The default build is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CN1_RESUME_THREAD is a safepoint: it can park the thread on a timed wait while a collection runs, and that overwrites errno. Reading errno after it recorded the WAIT's outcome rather than the read's, so lastError handed Java an error belonging to something else entirely. Captured at the syscall instead. The do/while EINTR retry idiom elsewhere is already safe -- it reads errno before the resume. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The mark phase signals every thread and spins until it answers, so it can scan the thread's native stack conservatively. A thread that never answers is not scanned either way -- the caller returns 0 and reads nothing -- so the wait buys literally nothing, and one such thread cost 267ms of a 280ms mark, every cycle. Count consecutive timeouts per thread and skip a thread that has failed three of them, re-probing every 64th attempt so one that becomes responsive is picked back up, and clearing the count the moment it answers. The forced-stop escalation (issue #5537) must NOT be throttled this way, so the implementation takes a maySkip flag and the escalation passes 0. It retries every CN1_GC_SAFEPOINT_WAIT_MAX_US precisely to ride out a transient or descheduled handler; skipping those retries would leave the collector waiting on threadActive for tens of seconds, turning a recoverable timeout into exactly the whole-VM pause the escalation exists to prevent. Measured on the server workload: stackMs 269 -> 0.20. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…sembly Two halves of one bug. Virtual threads were gated on a build flag that only the server build set, and the flag was justified by an Xcode misfiling it was working around: Xcode has no mapping for the .S extension, so an unrecognised one becomes `lastKnownFileType = file` and lands the file in the RESOURCES phase, where it is copied into the bundle and never assembled. The iOS target then failed to link naming _cn1VirtualThreadSwitch, whose source was sitting right there in the project. Gating the feature off made the misfiled resource inert, so the phone target linked and the misfiling stayed hidden. Fix the misfiling instead: .S maps to sourcecode.asm.asm (preprocessed, which the capability gate in the file needs) and .s to sourcecode.asm, and both route into the Sources phase rather than Resources. Every future assembly file gets this too. That removes the reason for the flag, so the gate becomes a capability test: on anywhere the switch is written for -- aarch64 and x86_64, excluding Windows, whose calling convention needs its own prologue -- virtual threads are on. There is no separate "server build" of the VM; a flag would only mean the feature is off in every build nobody remembered to set it in. Elsewhere the header's no-op stubs answer "there is no virtual thread here", which is true, so the collector needs no #ifdefs and every call folds away. CN1_DISABLE_VIRTUAL_THREADS forces that path. The predicate is repeated verbatim in the .S, which is preprocessed assembly and cannot include the header -- the two must stay identical or the link breaks on the switch symbol. Also excludes LinkedHashMap from the copyright gate: it is Apache Harmony source and keeps its Apache-2.0 notice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Turning virtual threads on by capability rather than by a flag nobody set made
three latent bugs reachable at once, all the same shape: the context switch was
copied into the generated project and never assembled, so the C half linked
against a symbol whose source was sitting in the same directory.
- CMake globbed *.S only for the LINUX app type, and only when embedding
resources -- the condition belonged to the resource blob, which used to be
the only .S there is. Now any .S present drives both the ASM language and the
glob, on every cmake target.
- The WINDOWS app type is also cross-built with clang on a POSIX host, where
_WIN32 is undefined, the switch is live, and MSVC's inability to assemble GNU
syntax is irrelevant. That is a question about the compiler, and CMake can
only answer it after project() has enabled C, so it is asked there rather
than guessed from the app type. Under MSVC the variable stays unset and
expands to nothing.
- Xcode has no mapping for .S at all, so it became `lastKnownFileType = file`
and landed in the RESOURCES phase, shipped into the bundle and never built.
sourcecode.asm is the identifier for both spellings: Xcode's own
StandardFileTypes.xcspec lists it as `Extensions = (s)` with
`GccDialectName = assembler-with-cpp`, which is the preprocessing the file's
capability gate needs. The neighbouring sourcecode.asm.asm is for .asm.
Tests. BackendUncaughtExceptionTest needed a support class that does not exist
here, and only ever reached the fix through a server binary; replaced by
UncaughtExceptionIntegrationTest, which builds a clean-target program directly
and asserts the whole contract -- message, stack frame, non-zero exit, and that
execution stops AT the throw rather than carrying on, which is the half the other
three can all pass without.
test_virtual_thread.c was built by nothing. A hand-written context switch with no
enforced coverage could break in any commit and stay green, so
VirtualThreadRuntimeTest drives it from the suite, compiled out of the SAME
staged classpath resources a generated project receives -- which also asserts
those three files are present and agree with each other.
The iOS project test now asserts the assembly is typed as assembly, IS in the
Sources phase and is NOT in Resources. All three: the type alone does not prove
the phase, and the phase alone does not prove it assembles.
The generator's own source set is what caught the last of it. Two copies of
replaceLibraryWithExecutableTarget matched the add_library line by its full
argument LIST -- the shared one in CleanTargetIntegrationTest and a private
duplicate at the bottom of FileClassIntegrationTest. Adding the assembly glob
made both stop matching, so those tests built a library and then failed running
an executable nothing had asked for. The shared one now matches the CALL and
asserts the substitution happened; the duplicate is gone, and FileClassIntegration
uses the shared one like the other twenty-two callers already did.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
All five were real. Taken together they are one theme: a virtual thread is a
mutator the collector cannot see by the usual means, and the code that creates
one was doing only half the job.
RUNNING VIRTUAL THREADS LOOKED PARKED. cn1SpawnVirtualThread builds its VM state
with bindToCallingOsThread false, which leaves threadActive FALSE, and nothing
ever raised it. A collection running concurrently therefore treated a mutator
executing Java as parked, and was free to scan or migrate its object stack and
pending-allocation table underneath it -- missed roots at best, corruption at
worst. The flag now moves with the context switch, up on resume and down on
suspend, because a SUSPENDED virtual thread genuinely is parked: the collector
reaches its roots through the registry snapshot instead.
The transition is a weak symbol with a no-op default, not a function pointer.
cn1_virtual_thread.c cannot include cn1_globals.h (the standalone runtime test
builds it with no VM at all), an indirect call on a path whose entire value is
that it costs 2.1ns is not free, and a weak symbol costs a direct call the linker
resolves to the VM's real one when there is a VM.
NOTHING RELEASED THE STATE. cn1VirtualThreadFree knows only about the coroutine.
The VM state spawned beside it holds a 264KB shadow stack, the call-stack arrays,
the pending-allocation table, and one of the NUMBER_OF_SUPPORTED_THREADS slots in
allThreads. A virtual thread per request would have consumed a slot per completed
request and eventually tripped CODENAME_ONE_ASSERT(threadOffset > -1). Added
cn1RetireVirtualThread, which marks the state dead the way an OS thread's death
does and then frees it with the same gcQueuedForDrain deferral the Java finalizer
uses.
THE UNCAUGHT-EXCEPTION EXIT WAS NOT GATED. This is the one that would have
shipped. The generated main() is emitted for every target that has one, iOS and
macOS included, and cn1AbortOnUncaughtException was set unconditionally -- so an
uncaught exception on any thread would have terminated a shipped app. The comment
sitting above it claimed the opposite ("Only this target opts in, so nothing that
ships today changes behaviour"), which was simply false: the enclosing guard is
`if(m.isMain())` and nothing more. Now gated on OUTPUT_TYPE_CLEAN.
BLOCKING STDIN NEVER PARKED THE MUTATOR. System.in.read() waits as long as nobody
types, with the thread left active, so a concurrent collection spun for a
safepoint that could not arrive until a human pressed a key. Bracketed with
CN1_YIELD_THREAD/CN1_RESUME_THREAD like the socket reads -- which then needs the
keep-alive those reads also need, because only an interior pointer into the array
is live across the call and the collector would otherwise sweep the buffer being
filled. Portable here (a volatile store) rather than the Linux port's asm
barrier, because this file also compiles under clang-cl. feof is read before the
resume for the same reason errno is: the resume is a safepoint, and anything
asked afterwards describes the wait.
THE SHADOW STACK WAS FREED THE WRONG WAY. cn1AllocThreadStack falls back to
calloc when mmap is out of MAPPINGS rather than out of memory, and
cn1FreeThreadStack always called munmap. That fails with EINVAL and leaks the
whole stack -- or, on an allocator that returns page-aligned blocks, unmaps
memory the allocator still believes it owns. Which allocator answered is now
recorded and the free is paired to it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… does
Mapper.Direct's contract is to produce exactly what
JSONWriter.toJson(toMap(instance)) would. Two fields did not, so a mapper changed
its wire representation on the day it gained a direct writer:
- A null List serialised as `null`, where the map path emits `[]` --
emitFieldToMap builds its ArrayList unconditionally and fills it only when
the source is non-null.
- Enum elements went through toString(). The map path uses Enum.name(), and
deserialisation matches against the declared constants, so an enum that
overrides toString() produced JSON that could not be read back at all.
Every other element kind was checked rather than assumed: appendJsonValue already
maps Date to getTime(), scalars and collections through writeJson, and a mapped
object through its own mapper -- the same three answers emitFieldToMap gives.
Nothing was comparing the two paths, which is why both got through. Every
existing test exercises one route or the other, never one against the other, so
the divergence was invisible to all of them. directJsonMatchesTheMapPathExactly
runs an object with a populated list, an enum list, a Date and scalars, and then
the same class with every list left null, asserting the two routes produce
identical text. It asserts equality of the paths rather than against a literal on
purpose: it keeps holding when a field kind is added, with nobody remembering to
extend a hand-written expectation.
Two things that test needed before it proved anything. It drives the generated
mapper's own toJson rather than Mappers.appendJson, which goes through the
registry -- unpopulated in an isolated classloader, so it fell back to toString()
and compared the map path against "com.example.Swatch@23706db8". And it asserts
the mapper actually implements Mapper.Direct, without which it would compare the
map path with itself and pass while testing nothing. The test enum deliberately
overrides toString() to disagree with name(), so the wrong choice cannot pass.
Also drops a redundant `public` on the interface: PMD's UnnecessaryModifier, and
a zero-findings gate.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ow needs Two CI breakages, both from this branch making something reachable that had not been reached before. EVERY cn1lib NATIVE CHECK STOPPED AT A MISSING HEADER. cn1_globals.h now includes cn1_virtual_thread.h -- CN1_RESUME_THREAD yields a virtual thread rather than sleeping the carrier it runs on -- and two places stage the port headers into a scratch directory to compile a cn1lib against them. Neither knew about the second file, so both stopped at "'cn1_virtual_thread.h' file not found" before compiling a line: the six ad-cn1lib xcodebuild probes and check-cn1lib-native-sources.py. The workflow's path filters gain the header too, otherwise a future change to it skips the very check that would catch this. java.io.File HAD NO WINDOWS PATH. Its non-ObjC arm is POSIX-only -- unistd.h, dirent.h, access(), X_OK -- and Windows reaches that arm under clang-cl, which is neither __OBJC__ nor POSIX. It went unnoticed because java_io_File_runtime.c is emitted only when an app actually uses java.io.File, and until the clean target became a usable program runtime no Windows build ever did. Now every one of them failed on 'unistd.h' file not found. The Win32 arm: io.h and direct.h for _access, the access-mode constants the MSVC CRT does not define, and FindFirstFile for the directory walk, in the same two-pass shape as the POSIX one (count, allocate, refill) because allocArray can collect and the array must not be built with a find handle open. X_OK maps to an existence check: Win32's access model has no execute bit, and _access REJECTS a mode of 1 rather than answering "not executable". isHidden asks for FILE_ATTRIBUTE_HIDDEN instead of guessing from a leading dot, which means nothing on Windows. Everything else -- stat, remove, rename, mkdir -- the CRT already provides under the same names. Also merges two identical project() branches that SpotBugs flagged as DB_DUPLICATE_BRANCHES: Linux and the clean target answer the assembly question the same way, so they share one branch instead of two spelled alike. The POSIX arm is verified here (FileClassIntegrationTest, 5/5); the Win32 arm can only be verified by CI, which is what reported it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CodeQL java/zipslip, high severity. unzip() built each output path by
concatenating the destination with ZipEntry.getName(), unchecked, so an entry
named "../../x" wrote wherever the archive asked. Both callers unpack a
DOWNLOADED zip -- Groovy for the console, JavaFX for the browser component -- so
the archive is not something the user authored, and the consequence is an
arbitrary file overwritten under their account while they believe they are
unpacking a dependency. CWE-22.
Every entry now has to resolve inside the destination or it is refused. The
comparison is between CANONICAL paths -- resolving the ".." is the whole point --
and it uses java.nio.file.Path.startsWith rather than String.startsWith, for two
reasons. Path compares COMPONENT-wise, so a sibling like "/tmp/dest-evil" is
rejected against "/tmp/dest" where a character-wise prefix accepts it, and giving
the string prefix a trailing separator to fix that then wrongly rejects the
destination directory itself. It is also the shape CodeQL recognises as a
sanitizer: the first attempt here was a correct canonical-path check that the
query still flagged, because a compound `!a && !b` guard did not read as a
barrier.
Two things the fix had to bring with it, both found by writing the test:
- Parent directories are created before extracting. FileOutputStream will not
create them, and a nested entry can arrive before the directory entry that
holds it, so "nested/deep/leaf.txt" in an archive that declares no directory
entries threw FileNotFoundException. That was broken before this change too.
- destDir uses mkdirs rather than mkdir, so a destination more than one level
deep is actually created.
Both streams are closed in a finally, which they were not: an IOException
mid-extract leaked the descriptor.
The test builds the malicious archive rather than checking one in -- a committed
zip that escapes its destination is an awkward thing to keep in a repository, and
building it puts the attack in front of the reader. Verified non-vacuous by
reverting the fix: 2 failures against the old code, 0 against the new.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Removing the unistd.h/dirent.h dependency got clang-cl past the first error and
into four more, all the same kind -- POSIX spellings the MSVC CRT does not have:
- `redefinition of 'timeval'`. <windows.h> pulls in <winsock.h>, whose timeval
collides with the one cn1_win_compat.h defines. WIN32_LEAN_AND_MEAN keeps
winsock out, and nothing here wants it.
- S_ISDIR / S_ISREG undeclared. The CRT has the st_mode BITS but not the macros
that test them, so they are defined from _S_IFMT/_S_IFDIR/_S_IFREG.
- PATH_MAX undeclared -- MAX_PATH is the Win32 spelling.
- realpath undeclared. _fullpath is the equivalent, but it takes
(destination, source), the REVERSE of realpath's (source, destination), so
the macro swaps them. Getting that backwards compiles and canonicalizes the
wrong string in silence. It also resolves a path that does not exist rather
than failing, which is the more useful answer for getCanonicalPath.
The POSIX arm is unchanged and still verified here (FileClassIntegrationTest,
5/5). The Windows arm is verified only by CI, which is what reported both rounds.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two Windows-only build breaks in this branch's own new code, both invisible on
the POSIX legs.
`i->gcPthread = 0` for a virtual thread's state is a type error under clang-cl:
pthread_t is a POINTER on Apple and glibc, but the Windows compat shim defines it
as struct {handle, id}, so the assignment reads as "assigning to 'pthread_t' from
incompatible type 'int'". memset over sizeof is correct for both shapes, and
gcPthreadValid -- set FALSE on the next line -- is what actually gates every read
of the field.
cn1AllocThreadStack declared its byte count above the #if that uses it, so on
Windows, whose arm calls calloc with the element count instead, it was an unused
local. Moved onto the arm that uses it.
Swept the rest of this branch's additions for the same class of thing rather than
waiting for CI to find them one at a time: every other POSIX call in code Windows
compiles is either guarded (mmap/munmap behind !_WIN32, pthread_attr_setstacksize
behind __linux__) or shimmed in cn1_win_compat.h (usleep, pthread_key_create,
pthread_getspecific). The virtual-thread runtime -- including the
__attribute__((weak)) definition, which clang-cl treats differently on COFF -- is
entirely inside the CN1_VIRTUAL_THREADS gate, which excludes _WIN32, so none of
it is compiled there at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A null array crashed instead of throwing (P1). CN1_ARRAY_STORE_CHECK evaluates CN1_CLASS_OF(arrayObj) with no null guard, and under -Dcn1.checkedCasts it runs AHEAD of the setter that turns a null array into a NullPointerException -- so an object-array store through a null array took the process down. Java orders NPE ahead of ArrayStoreException anyway, so falling through to the setter is both the safe answer and the correct one. A virtual thread's stack could go unmarked mid-switch (P1). The parked-stack pass skipped anything cn1VirtualThreadIsRunning() reported, on the reasoning that the carrier covers those. It does -- but only once the carrier's stack pointer is actually INSIDE the virtual stack, and `running` is raised before the switch and lowered after the switch back. In those two windows a stopped carrier still has an OS-stack pointer, so cn1VirtualThreadForStackAddress matches nothing, the carrier pass scans only the OS stack, and this pass skipped the virtual stack for being "running". References held in C temporaries there could be swept. The flag cannot be made atomic with the switch it brackets, because the switch is what changes the stack the flag would have to be written from. So the passes now OVERLAP instead of partitioning: every virtual thread's saved region is scanned unconditionally. Safe, because [sp, stackHigh) is inside the mapping whenever sp is non-zero; complete, because while a virtual thread runs the carrier's pointer is lower, so this pass covers a subset and the carrier covers the rest; and cheap, because conservative marking is idempotent. cn1RetireVirtualThread's "use after free" was NOT one, and the code now says so. markDeadThread -> collectThreadResources sets gcQueuedForDrain unconditionally and has no early return, so the synchronous release branch was unreachable. It read as live, though, so it is gone and the invariant is written down -- including the reason it matters, which the report had right: codenameOneGCMark copies each ThreadLocalData* out of allThreads under the critical section and dereferences it OUTSIDE the lock, so a synchronous free would be a genuine use-after-free. File.list returned something that called itself a String. All three arms passed the ELEMENT class to allocArray, which installs whatever it is given as the array object's own class; cn1MainArgs has always passed class_array1__java_lang_String. Pre-existing on iOS and Linux, copied into the new Windows arm, fixed on all three. Windows absolute paths were treated as relative, which corrupted them rather than merely misreporting them: getAbsolutePathImpl tested p[0] == '/', so "C:\data" had the working directory prepended. There is now a per-platform predicate that knows about drive letters and UNC roots. The matching Java-side gap is deliberately left and documented at the predicate: File.isAbsolute() tests startsWith(File.separator) and separator is "/" everywhere, which needs a per-platform separator in shared JavaAPI -- a change for every port, not for making the clean target build. Blocking file reads and writes now park the mutator, like the socket reads and StandardInputStream already did: a FIFO, a device or a network-backed path blocks for as long as the far end stays quiet, and an active thread there strands the collector waiting for a safepoint that cannot arrive. Both carry the buffer keep-alive for the same reason those do -- only an interior pointer is live across the call. (Moving that macro above its first use is why it now sits at the top of the file layer rather than beside stdin.) The benchmark helper compiles the emitted .S. Third place with this bug: the CMake generator and the Xcode project generator had it too, and a *.c-only invocation links against a missing cn1VirtualThreadSwitch on any target where the switch exists. Two findings are recorded in the file rather than fixed, with the analysis and the actual remedy: 32-bit ftell/fseek cannot express a position past 2GiB where C long is 32 bits, and paths reach the narrow CRT as UTF-8 and are read as ANSI. Both are pre-existing on every platform, both want a change across the whole file layer, and neither is what enabling the clean target is about. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Mapper.Direct promises identical output, not better output. Each of these was the
direct path being reasonable in a way emitFieldToMap is not, which is the same
thing as changing a mapper's wire format the day it gains a direct writer.
- A property NAME was escaped for the Java literal and not for JSON. escape()
doubles a quote so the generated source compiles; the resulting writer then
appended the raw character, so a @JsonProperty holding a quote emitted
"a"b" -- unparseable. The map path never had this because JSONWriter puts the
key through writeString. Now jsonEscape composed with escape: one makes the
JSON valid, the other makes the source compile. Done at generation time, since
a jsonName is a compile-time constant and the writer should stay a literal
append.
- A Property value was rendered too well. emitFieldToMap stores it RAW, so
JSONWriter renders a Date or a mapped object through String.valueOf;
appendJsonValue turned them into epoch millis and nested JSON. New
Mappers.appendJsonRaw is exactly JSONWriter's answer for a value that was put
in the map unchanged.
- A reference field looked its mapper up by RUNTIME class. A field declared as a
mapped base holding an unmapped subclass therefore found nothing and fell back
to a quoted toString, where the map path asks Mappers.get(Declared.class) and
serialises it as an object. New Mappers.appendJsonUsing takes the mapper the
caller names, and still uses that mapper's direct route when it has one.
- Mapped list ELEMENTS had the same problem, plus the general one behind it: the
direct path had a two-way branch where emitFieldToMap has four. It now mirrors
them one for one -- enum name(), scalar raw, Date getTime(), everything else
through the declared element type's mapper.
The test was the actual defect. Nothing compared the two paths against each other,
which is why all of this shipped; and the parity test added for the first pair
needed three fixes of its own before it proved anything:
- It went through Mappers.appendJson, which consults the registry. In an
isolated classloader the registry is empty, so it compared the map path
against "com.example.Swatch@23706db8". It now drives the generated writer.
- The polymorphic case had no mapper registered for the base type, so BOTH paths
fell back to toString and agreed. Registering it is what makes the two
implementations able to differ at all.
- assertEquals reports the FIRST difference, so one unfixed case masked the
others. Each representation is now pinned individually, which also catches the
case equality cannot: both paths wrong in the same way.
Verified by reverting the generator with the test in place: one failure against
the old code, six passing against the new.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two more review findings, both in code this branch touched. skip(Long.MAX_VALUE) computed `start + count` and clamped afterwards. Once any byte has been read that addition overflows signed long -- undefined behaviour, and in practice a wrap to negative, so the seek goes BACKWARDS and the caller is told it skipped a negative distance or gets an error where it should have landed on EOF. It now clamps against the remaining DISTANCE, which cannot overflow: end is at least start, and start plus the clamped amount is at most end. File.list walked the directory TWICE -- count, allocate, walk again -- and assumed both walks saw the same directory. They do not. A file created in between overruns the array, and CN1_SET_ARRAY_ELEMENT_OBJECT turns that into ArrayIndexOutOfBoundsException; a file removed leaves trailing nulls in a String[] that no caller expects. Directories change under readers routinely, so this was never sound. I wrote the Windows arm that way deliberately, mirroring the POSIX one, which means I copied the structure without asking whether it held. Both arms now enumerate ONCE into a small growable list of names and build the array afterwards. The names are held in C memory on purpose: allocArray and newStringFromCString can both collect, and nothing may hold a directory handle across that. The ObjC arm is left alone -- NSFileManager hands back a snapshot, so it never had the race. Also moves stdlib.h to the shared include group, since the list uses malloc/realloc/free on both arms and sits outside the platform blocks. The test is the part worth reading. FileClassIntegrationTest never called File.list(), so the native listing was COMPILED but never RUN by any suite: the rewrite above passed 5/5 while executing none of it, and reverting it would have passed too. Coverage now creates a directory, lists it, and pins the three things that were wrong or fragile -- the entries, the absence of nulls, and that the result is a String[] rather than a String, which is the pre-existing allocArray class bug nothing had ever asserted. Confirmed the assertions discriminate rather than merely execute: with the array class reverted to the element class, all five configurations FAIL; restored, all five pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two more findings, both consequences of this branch making java.io.File usable on Windows. "C:foo" is DRIVE-RELATIVE: relative to the working directory of drive C, which is not the process working directory and may be on a different drive. cn1FileIsAbsolute classified it correctly -- the comment there even says so -- and then the fallback prepended the process cwd anyway, producing "D:\cwd\C:foo", which names nothing. The predicate knew about a case the code after it did not. _getdcwd asks the right drive. Deliberately not _fullpath, which the report suggested: it also normalises "..", and getAbsolutePath is specified NOT to do that -- resolving is getCanonicalPath's job. Using it would have swapped a wrong path for a subtly wrong contract. createNewFile was check-then-act: access(), then fopen(p, "w"). Losing that race does not merely return the wrong answer, it TRUNCATES the file the other process just created, and then reports true as though it had done the creating -- which is exactly the failure mode the lock-file and single-instance patterns it exists for cannot survive. Now a single O_EXCL open on both arms, with the kernel deciding. Pre-existing on POSIX too, so both are fixed. ON THE TEST, because the distinction matters: the coverage added here is a REGRESSION GUARD, not a demonstration of atomicity. It checks the uncontended path -- createNewFile on an existing file returns false and leaves it intact -- and the old check-then-act version passes it too, because access() succeeds and it returns before reaching the truncating fopen. Confirmed by running the suite against the old implementation: 5/5 green. The real defect needs a file to appear between the check and the open, which one thread cannot arrange, so the argument for the fix is structural rather than empirical and the comment in the test says so. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
newStringFromCString turns each byte into its own char. That is correct for what it exists to serve -- generated string literals, which are ASCII plus ~~uXXXX escapes -- and wrong for anything arriving from outside the program. A UTF-8 "e-acute" is two bytes, so main(String[]) and System.getenv handed back one garbage char per byte, corrupting paths and option values before the program had a chance to look at them. Both entry points are new in this branch. newStringFromUtf8 decodes properly: multi-byte sequences, surrogate pairs for astral code points, and U+FFFD for malformed input the way java.lang.String's own decoder does -- a program should not die because one environment variable holds a stray byte. Overlong forms, UTF-8-encoded surrogates and out-of-range code points are all rejected. newStringFromCString itself is deliberately NOT changed. Every native-to-Java string in the VM goes through it, its byte-widening is load-bearing for the literals it serves, and its own comment records that the high-bit path is bit-identical to what came before. Correcting the two entry points this branch added is the scoped fix; the general version is the same work as the ANSI-versus- UTF-8 path issue already recorded in nativeMethods.m. TWO BUGS UNDERNEATH, both found by the test rather than by reading: newString was broken and had never been called from C. JAVA_CHAR is an int and JAVA_ARRAY_CHAR is an unsigned short, and it sized the allocation with sizeof(JAVA_CHAR) while memcpy'ing length * sizeof(JAVA_ARRAY_CHAR) bytes out of a four-byte-element array -- half the input, at the wrong stride. My decoder was its first caller and hit it immediately: "cafe" came back as c,NUL,a,NUL,f. It now narrows element by element. Behind that, the representation is not a free choice. A string whose units all fit in a byte is stored as a COMPACT byte[], anything else as a char[], and charAt reads whichever it finds -- so handing it the wrong one reads 8-bit units out of 16-bit data and produces exactly the same symptom rather than failing. That rule now lives in cn1StringFromUnits, used by newString and newStringFromUtf8. newStringFromCString keeps its own copy on purpose: it tracks the Latin-1 flag during decoding and runs for every literal at startup, so routing it through a helper that recomputes would add a pass over every literal in the program to save a dozen lines. The comment says so, and says the two must change together. The test reports CODE POINTS rather than text, so it cannot pass through a console-encoding coincidence: "cafe-acute-euro" must arrive as 99,97,102,233,8364, which covers a two-byte and a three-byte sequence. Byte-widening reports the individual bytes instead, which is how the newString bug surfaced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s UTF-8 The Windows clean-target leg failed the test added with the UTF-8 decoder, and it was right to: "cafe-acute-euro" arrived as 99,97,102,65533,65533 -- c, a, f, and two replacement characters. The CRT hands main() and getenv() the wide command line and environment already converted down to the ACTIVE CODE PAGE, so decoding those bytes as UTF-8 finds invalid sequences and substitutes U+FFFD for every non-ASCII character. That failure was predicted by a comment I had written in this very function -- which then shipped alongside a test asserting the behaviour the comment said did not exist. MultiByteToWideChar with CP_ACP is the conversion Windows actually needs, and it yields UTF-16 code units directly, so nothing decodes afterwards. RENAMED from newStringFromUtf8 to newStringFromNative for the same reason: a function named FromUtf8 that deliberately does not decode UTF-8 on one of its platforms is a trap for whoever reads it next. The name now says what it does -- convert text that came from the OS, in whatever encoding the OS used. WIN32_LEAN_AND_MEAN before windows.h, which is the same winsock timeval collision that broke java_io_File.m; and the byte-length local moved onto the POSIX arm, which is the only one that uses it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The JLS orders these: NullPointerException, then ArrayIndexOutOfBoundsException, then ArrayStoreException. Under -Dcn1.checkedCasts the emitted covariance check ran BEFORE the setter that reports the first two, so a store with both a bad index and an incompatible value reported the value -- hiding the exception the program should have seen. (The null case was worse and is already fixed: the check dereferenced the array to reach its class.) The store check is now guarded by the same access validation the setter performs, so the first two exceptions are thrown first and in the right order. The setter re-checks, which on the in-bounds fast path costs one comparison. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e collector This backs out my own fix from earlier in this branch. Marking the attached ThreadLocalData threadActive around the context switch reads as obviously correct and is a REGRESSION, worse than what it fixed. A virtual thread's state has no pthread of its own -- deliberately, it may run on a different carrier next time. The collector's wait for a lightweight thread is `while(t->threadActive) usleep(500)` with no bound, and the forced-stop escalation that exists to break exactly that wait is gated on gcPthreadValid, which is permanently false here. So the flag converts a POSSIBLE race on the state's object stack into a CERTAIN hang for any virtual thread that computes without reaching a safepoint: the collector waits for a flag only that thread can clear, and cannot stop it. What the same report asked for has two halves, and the other one stands. The C stack is covered: cn1GcScanParkedVirtualThreads scans every registered virtual thread whether or not it is running, so no virtual stack goes unscanned during the windows where `running` is set but the carrier has not switched yet. That fix is independent of this revert and stays. The half that remains open -- a collection walking the state's object stack and pending-allocation table while the virtual thread mutates them -- is documented at cn1SpawnVirtualThread along with why the obvious fix is worse and what the real one is: carrier association. A running virtual thread executes ON a carrier that does have a stoppable pthread, so the collector should satisfy the wait by stopping the carrier. That needs the stop handshake to stop being per-TLD (the signal handler records into the TLD of the thread it runs on, which is the carrier's), i.e. a change to the collector's stop protocol rather than to the spawn path -- not something to improvise in an API that has no callers yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
With -Dcn1.checkedCasts the covariance check broke correct programs, which is the worst direction for a check to fail in. A generated array class records arrayType as the BASE element class rather than the immediate component: String[][] has dimensions 2 and arrayType String, not String[]. So `values[0] = new String[1]` asked whether a String[] is an instance of String, got no, and threw ArrayStoreException on a store the language requires to succeed. Restricted to dimensions == 1, where arrayType genuinely IS the component type. Multidimensional stores lose a diagnostic that did not exist before this feature was added; the alternative was breaking working code. Covering them properly needs the immediate component type, either emitted per array class or reconstructed from dimensions at runtime, and the macro says so. Also fixes a timeout in VirtualThreadRuntimeTest that could never fire. It read the child's output inline and then called waitFor: the read blocks until the child closes stdout, so a binary that hangs -- exactly what a context-switch regression produces -- never reached the timeout, and the Maven job would sit until CI killed it instead of the test failing. Output now drains on its own thread, with a bounded join so a wedged reader cannot reintroduce the hang the change removes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This corrects my own change earlier in this branch, and the reasoning behind it was the defect. "A thread it cannot stop is one it does not scan either way" is true only while the thread genuinely cannot be stopped. Failures are often TRANSIENT -- a stop signal briefly masked is enough -- and the thread recovers. Skipping it then meant cn1GcScanThreadNativeStack returned without scanning a RESPONSIVE thread, for roughly the next sixty collections, so references held only in frameless C locals or registers went unmarked and could be reclaimed while still in use. A GC correctness bug, traded for a performance win. The two things I had conflated: the cost was never the SIGNAL, it was the WAIT. One unresponsive thread consumed the entire 2,000,000-spin budget -- 267ms of a 280ms mark. So a thread with a failure history is now probed with a 20,000-spin budget rather than skipped. Healthy threads answer within about 200 spins, which is a hundredfold margin for one that is merely slow, at one percent of what a hang used to cost; and a thread that recovers is picked up on the very next cycle instead of up to 64 later. Verified across the GC suites, including GcUncooperativeThreadIntegrationTest -- the issue #5537 scenario this logic exists to serve: 6/6. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Boolean shares its kind with boolean and Character with char, so the direct writer treated both as primitives. Only the boxed form can be null, and both handled it wrongly in opposite ways: a null Boolean was unboxed by a ternary and threw NullPointerException, and a null Character went through String.valueOf(Object), which returns the four characters "null", and was then QUOTED -- so an unset field serialised as the string "null". The map path stores the value and lets JSONWriter see the null, emitting JSON null for both. Told apart by binaryName, which does distinguish them, with a temporary in each so a getter is not evaluated twice, and charValue() so String.valueOf resolves to the char overload rather than the Object one. The parity test carries both fields now, and they discriminate by construction: against the old code the Boolean case throws (a test error) and the Character case produces a quoted "null" against the map path's null (an assertion mismatch). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CODENAME_ONE_ASSERT is plain assert(), which NDEBUG compiles out of every release build. So once all NUMBER_OF_SUPPORTED_THREADS slots were taken, threadOffset stayed -1, the assertion vanished, and the next statement executed allThreads[-1] = i -- writing over whatever precedes the table. A debug build aborted; a shipped one carried on with silent memory corruption, which is the worse of the two. Capacity exhaustion is a condition to report, not to assert. It returns 0 now, and cn1SpawnVirtualThread already checks for that. Pre-existing rather than new: every OS thread creation runs this path too. A virtual thread per request only makes reaching the limit realistic. The partially built state is unwound through cn1FreeThreadLocalDataFields, extracted from cn1ReleaseThreadLocalData rather than copied, because the release path also decrements nThreadsToKill and a state that never reached allThreads was never counted as living. Duplicating the frees would have drifted apart, and getting that counter wrong would have been a slow leak in the opposite direction. Verified across the GC suites including GcUncooperativeThread and GcHeapIntegrity: 6/6. (The translator build says nothing about this -- it compiles Java, and the C here is only compiled by those tests.) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…reeing Two defects, and both are mine from earlier in this branch. THE HANG I REVERTED WAS STILL REACHABLE. Removing the threadActive assignment from cn1VirtualThreadResume did not close it, because CN1_RESUME_THREAD does the same thing and every bracketed native goes through that macro. getThreadLocalData() resolves to the VIRTUAL thread's state while one is running, so a virtual thread that read a file or a socket returned with its state marked active, and nothing lowers it again until the next yield. Same unbounded while(threadActive) wait, same forced-stop escalation gated on gcPthreadValid and therefore unavailable, same stall. I checked the call site I had edited and not the shared path through it. The guard states the invariant the code always needed: mark active only what the collector can STOP. gcPthreadValid is exactly that question. A real thread is unaffected; a virtual thread's state stays down, which is where it was before any of this. Roots do not depend on the flag -- cn1GcScanParkedVirtualThreads scans every registered virtual thread whether or not it is running. THE EXHAUSTION CHECK INTRODUCED A USE-AFTER-FREE. pthread_setspecific binds the new state to TLS above the capacity search, so the failure path I added freed a state the key still pointed at: every later getThreadLocalData() on that thread would return memory that had been given back. That is worse than the out-of-bounds write it replaced, because the thread keeps using the stale pointer rather than failing. Unbound before the free. Also: System.getenv(null) throws NullPointerException as the API requires, instead of returning null and making an invalid argument indistinguishable from an unset variable. Verified across the GC suites, 6/6, including GcUncooperativeThread and GcHeapIntegrity. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c18e9264e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The packaged arm caches one OpenSSL SSL_CTX per CA path, so a bundle is parsed once rather than on every handshake. Keying that cache on the path alone was wrong: a CA bundle is a mount, and mounts are rotated under a stable name -- a Kubernetes secret or configmap, cert-manager, an RDS bundle refresh. The path never changes, so a long-lived backend went on trusting the roots it read at startup, and every connection failed the moment the database or service presented a certificate signed by the new one, with a process restart as the only remedy. The Java SE arm builds its trust factory per upgrade and never had this, so the two arms disagreed about a deployment that is meant to be routine. Each cached context now carries the identity of the file it was built from -- st_dev, st_ino, st_size and mtime to the nanosecond. Inode and device catch the atomic-rename form (Kubernetes swaps a symlink, and stat follows it), size and mtime catch a rewrite in place, and the nanoseconds matter because a bundle rewritten within one second at the same size and inode is otherwise indistinguishable. Build first, swap second. Every failure path returns with the existing entry untouched, so a rotation caught halfway -- the file replaced but not yet readable, or briefly truncated -- keeps serving the old context instead of emptying the slot and handing the next caller a null. The stamp is written only for a context that loaded, so the rebuild is retried until one does. Dropping the cache's reference is safe while handshakes are in flight: SSL_new took its own, so an SSL still using the old roots finishes on them. SelfTest covers it on both arms by copying a real bundle, rotating it in place to a PEM that cannot load, and rotating back. Rotating good to BROKEN is what makes it decisive -- a failed build is not cached, so good-to-good would pass either way. A/B'd: with the stamp check removed, "a rotated CA bundle is re-read" reports verified instead of refused. The harnesses derive the bundle path instead of only passing the variable through, because a check that needs an environment variable nobody sets never runs and reads as green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4246302fc2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
CN1_HTTP_TIMEOUT_MS was read unclamped, and a non-positive value undoes the guarantee the setting exists for: the Java SE arm reads it as "no deadline" and the packaged one disables SO_RCVTIMEO, so a connection that opens and says nothing holds a worker forever and the pool is bounded on purpose. It now takes the same envIntAtLeast the minimum body rate does. A NEGATIVE value was worse than useless. setsockopt fails, and it failed inside acceptAll BEFORE the descriptor reached liveConnections -- where the handler routed it to drop(), which returns without closing a descriptor it does not own. One leaked fd per connection, for as long as the option kept failing. The accept path now closes a descriptor that was rejected before registration, directly rather than through drop(), which also covers any other setsockopt failure rather than only the one the clamp removes. The three other setBlocking sites were checked and are mid-connection, where drop() does own the descriptor. StaticFiles mounted at /assets served the index as 200 for a request to /assets. Stripping the prefix leaves an empty target, which became "/" and then "/" + indexFile and resolved to a FILE -- so the directory branch that exists to issue exactly this redirect never ran. A browser then resolved "style.css" against / rather than /assets/, and every relative reference in an otherwise valid site pointed one level too high. The empty post-prefix path now redirects to the directory form, carrying the query, the way the branch below it already does. Both A/B'd. Without the clamp the fixture logs no refusal; without the redirect, GET /static answers 200 with the index body. The leak itself is closed by construction rather than by a test -- with the clamp in place the env var can no longer make setsockopt fail, which was its only reachable trigger. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The shape test pinned all 29 characters except the first three, so the weekday was accepted whatever it said: "Xxx, 06 Nov 9999 08:49:37 GMT" parsed to a year-9999 timestamp, which StaticFiles.isNotModified() reads as newer than any file and answers 304 -- a conditional request served no content because its date was nonsense. Measured before the fix: 253397494177000. The name is checked AGAINST THE DATE rather than only against the seven. daysFromCivil has already run by then, so the stronger test is free, and a day-name inconsistent with the date is something RFC 9110 requires a sender not to produce. Rejecting it is the safe direction -- the answer is a full response rather than a 304 -- and every date this file emits is consistent by construction, so a client echoing our own Last-Modified back can never trip it. That closes the last unvalidated span of the format: weekday, separators, day, month, year, hour, minute, second and the GMT suffix are now all tested. A/B'd on both arms with the check disabled -- the unknown name returned the year-9999 value above and "Mon" for a Sunday returned 784111777000. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 862a4b9fbf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
isHeaderSafe rejected CR, LF and NUL as CHARS, but both writers narrow with a plain cast -- Buffer.put does out[n] = (byte)charAt(i), and asciiBytes the same. U+010A is not '\n' to a char comparison and is byte 0x0A on the wire, so a handler reflecting a query parameter into a header could be made to write a real newline into the header block. That is response splitting, and a cache-poisoning primitive -- the exact defect the comment beside the caller says is being prevented. Measured before the fix, with ?v=%C4%8A: the reply ended "X-Reflected: " followed by a bare LF. The rule is now RFC 9110's field-value -- HTAB, SP, VCHAR and obs-text -- tested against the byte that will be emitted, which also answers the narrower finding that only CR, LF and NUL were refused among the C0 controls and DEL. Anything above 0xFF cannot be spelled in one byte and is refused rather than narrowed into whatever it happens to alias. The h2 path built a Request straight from :method, so the two protocols on one server disagreed: "BREW" and lowercase "get" were 501 over HTTP/1 and reached the handler over h2, where the test measured a 200. Both now answer 501, and the test asserts the two protocols agree rather than checking h2 alone. isKnownMethod already existed, hand-listing the same seven methods as KNOWN_METHODS and with no callers at all -- so a method added to KNOWN_METHODS would have been routed by the HTTP/1 parser and refused by it, with nothing to notice. Deleted in favour of one derived from the array. /rawheader now reflects a query parameter, which is the shape the value rule exists for and the only one a client controls; the four malformed names beside it are the handler's own. Both fixes A/B'd. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae3447cca1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Web hands its headers to the packaged native as ONE string with '\n'
between them, and the native splits on that byte and gives each line to
libcurl. Nothing validated them, so a value derived from untrusted input
-- the bearerToken getJson() and postJson() accept is exactly that --
became an extra header the caller never wrote: "abc\nX-Admin: true" adds
a second header to a request the upstream trusts.
Both arms now validate before either HTTP stack is reached, through one
shared HeaderLines in the runtime tree. Leaving it to the stacks is how
they disagree, and measuring that is what settled the design: with the
check removed, the Java SE arm refused the LF spelling (the JDK's "Illegal
character(s) in message header value") and accepted CR, U+010A and a name
with a space in it, all of which went out. So the arms already disagreed
about four of the five spellings, and only one of them was anybody's
deliberate rule.
Separately, the HTTP/1 parser settled WHICH authority a request carries --
missing Host, duplicate Host, a target authority disagreeing with the Host
-- and never whether that authority is well formed. "Host: user@internal"
reached the handler with a 200, measured. An application routing or
authorizing on getHeader("host") then acts on a value a conforming proxy
in front of it would have rejected; proxy and origin disagreeing about the
authority is the Content-Length/Transfer-Encoding problem wearing a
different header. The absolute-form authority is checked too rather than
relying on the equality test to carry the verdict, because that test runs
only when both are present and an HTTP/1.0 absolute-form request has no
Host to compare against.
Also routed pr.yml's three remaining raw apt-get calls through
scripts/ci/apt-get-install.sh. "timeout 300 apt-get update" turns a
stalled mirror into a FAILED job rather than a hung one, which is all it
ever bought -- and it just did, taking build-test (8) down with it, and
the quality-report step after it for want of the reports that step never
reached. The wrapper prunes the broken vendor sources before updating,
forces IPv4, retries once, and skips the round trip entirely when the
packages are already present -- which is the usual case here, since all
three sets are baked into the CI container image.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc2ad85676
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two contract-generator divergences, both of which made one request mean different things depending on which generator served it. The generated dispatcher required the verb to equal GET exactly, so a HEAD answered 404 where the same path through @RestController's router answered normally. RFC 9110 defines HEAD as GET without the content, HttpServer routes it and strips the body itself on both protocols, so the handler needs to know nothing about it. There is nothing to conflict with either: the contract annotations are Get/Post/Put/Delete/PatchMapping, so a HEAD route cannot be declared. The generated query helper required an '=' and answered null for "?flag". HttpServer.Request.queryParam answers "" for that form deliberately -- it is the flag spelling -- so identical bytes bound as absent through a contract and as present-and-empty through a @RestController, which reaches required-versus-default handling and primitive conversion before the handler sees anything. THE THIRD FINDING WAS WRONG, and the tests say so rather than the commit message alone. Review read `ifNoneMatch.indexOf(etag) >= 0` as taking "prefix10-20" for "10-20". It does not: the tag is built WITH its quotes three lines above the call, so the needle includes both of them and in "prefix10-20" the opening quote is followed by 'p'. An ETag cannot contain a quote either, so no other validator's quoted form can embed ours, and for well formed input the substring test agreed with list membership everywhere. Measured: the new cases pass unchanged against the old line. Replaced anyway. Being right for that reason is being right only while the tag stays quoted, and whoever later writes an unquoted validator turns a correct line into a cache-poisoning one without touching it. The list reader is the same work and depends on nothing three lines away. It keeps weak comparison, which is the one If-None-Match takes, and answers false for a malformed field -- sending the representation rather than a 304 the client cannot undo. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46a6be682e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The authority validator added a commit ago allowed '%' as an ordinary reg-name character, so "Host: bad%zz.example" passed it. pct-encoded is "%" HEXDIG HEXDIG, and an authority a conforming frontend rejects or normalises is exactly the proxy-versus-origin disagreement that validator exists to close -- reintroduced one level down, inside the fix for it. Both branches take the rule now. The bracketed form has no use for a percent except the "%25" that introduces an RFC 6874 zone id, and that is two hex digits like any other. Hex.digit is the same strict reader the rest of this tree uses for an escape, so "%4" and a triplet that runs off the end are refused along with "%zz". Measured before the fix: "Host: bad%zz.example" answered 200. A complete triplet is covered on the accept side too, because a validator that refuses what it should accept is the failure mode this one is one edit away from. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e1e9be87a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
"?name=%C3%28" is well formed hex and a TRUNCATED two-byte sequence.
new String(_, "UTF-8") answers U+FFFD rather than failing, so the handler
received U+FFFD followed by '(' and the request became indistinguishable from
"?name=%EF%BF%BD%28", which spells that value legitimately. A frontend
that validates UTF-8 rejects one and passes the other, so the two ends
disagree about what arrived -- the same shape as a malformed Host, and the
same defect this PR already refused for a request BODY and for a static
file path. The target was the hole that was left.
Refused at parse time rather than in queryParam, which returns a String
and cannot say "malformed": answering null there would make a bad
parameter look absent, which is worse than either answer.
Deliberately NOT a rule about malformed ESCAPES. percentDecode passes
"%zz" through as literal bytes and browsers do send a bare '%';
"?pct=100%25andmore" is covered on the accept side to keep that so. The
validator decodes exactly as percentDecode does, so the two cannot
disagree about what the handler would have seen, and a target with no '%'
in it returns on the scan without allocating -- which is every request
that has no escapes.
Measured before the fix: "?name=%C3%28" answered 200.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8298b4947
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A USE AFTER FREE I INTRODUCED with the CA-rotation fix. Handing back the cache's raw SSL_CTX* was safe only while contexts were never freed. Rotation made them mortal: between the unlock and the caller's SSL_new, another thread can notice the same bundle changed, rebuild the slot and drop the cache's reference -- the only one -- so the first thread passes freed memory to OpenSSL. The reference is now taken under the same mutex that guards the slot and released the moment SSL_new has taken its own, which is one place rather than each of the seven error paths below it. Proved rather than argued, with AddressSanitizer over the rotation check: with the retain removed the run dies on "attempting double-free ... in SSL_CTX_free", and with it the same run is clean at 165/165. The race itself needs two threads; the refcount error it turns into does not, which is what makes it testable at all. Separately, the UTF-8 target check added last commit was on the HTTP/1 parser only, so "?name=%C3%28" was refused over HTTP/1 and served over h2 -- one server, two answers, which is the defect the check exists to stop rather than an oversight beside it. The h2 dispatch now applies the same rule. Its :path arrives already decoded into a String, so it is re-encoded to UTF-8 first: literal non-ASCII text came from valid bytes and re-encodes to valid bytes, while the percent escapes, the only part that can be malformed, are checked exactly as on the HTTP/1 side. Found by sweeping the paired implementations for this rather than waiting for it to be reported -- the same sweep cleared the javase/parparvm Crypto pair, whose apparent asymmetry is the native doing the checking (cn1_backend_crypto.c:119). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5706448f02
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
:authority IS Host over h2 -- this server copies it into the
handler-visible "host" field -- and it was copied there unvalidated while
the HTTP/1 side refused the same value. A handler reading
getHeader("host") cannot tell which protocol carried the request, so that
is one server giving two answers, and host-based routing or authorization
is what acts on the difference. RFC 9113 8.3.1 also requires :authority
and a Host field to agree when both are sent, which the HTTP/1 parser
already enforces for a target authority against Host.
WHAT THE TEST ASSERTS IS WHAT MEASUREMENT SHOWED, and it is not the whole
finding. nghttp2 applies its own messaging validation first: measured,
":authority: user@internal" -- the example review named -- and one
containing a space are refused at the stream level with no response at
all, so they never reach this code and cannot be asserted as a 400. The
ones that DO arrive are "example.com:notaport", "bad%zz.example" and
":8080", and they reached the handler before this change:
example.com:notaport answered 200.
Separately, fromMapList answered null for a scalar or an object where an
array was declared, so a malformed shape was indistinguishable from an
explicit JSON null and the handler ran with a null field -- while the
element check three lines below it already threw the
IllegalArgumentException the transport turns into a 400 for the same
mistake one level down. A container of the wrong type is no more the
client's prerogative than an element of the wrong type. Null stays null
for an actual null, which is covered on the accept side.
fromValueList already had the rule. The emitted asList helper, which had
the same silently-nulls shape, had no callers in the generated code and is
removed rather than left as the next place to reintroduce this -- the
generated sources still compile in all 36 tests, which is what proves it
was unused.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf49bac5f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The database URL decoder wrote a literal character into a byte buffer with
a plain cast and then read the buffer back as UTF-8, so "p<a-umlaut>ss%40word"
put byte 0xE4 -- a lead byte with no continuation -- where two bytes
belonged, and the password decoded with U+FFFD in place of the letter. The
credential, database name or CA path silently became a different string.
Only reachable when one component carries an escape AND a literal
non-ASCII character, which is why an accented password on its own always
worked and this stayed invisible until something else needed encoding.
Literals are now gathered and handed to the platform encoder rather than
encoded by hand, so a surrogate pair comes out as the one four-byte
sequence it is instead of two malformed halves. Covered through sslmode,
the one decoded value this API repeats verbatim, so the decoder's output
is observable without a live server and without printing a password.
Range bounds took Long.parseLong, which accepts a sign and the whole of
Character.digit's repertoire. MEASURED, because the two arms do not agree
and the reported example only holds on one of them:
input JDK (Java SE arm) vm/JavaAPI (packaged)
"+0" 0, read as a range NumberFormatException, ignored
Arabic-Indic zero 0 0, read as a range
So the sign was a hole on the Java SE arm only -- vm/JavaAPI's parseLong
special-cases '-' and not '+' -- while the Unicode digits were a hole on
both, and that is the case the A/B caught. Each bound is now an unsigned
run of ASCII digits or absent, which also drops a .trim() that accepted
whitespace RFC 9112 does not allow inside the spec.
Being strict is safe here precisely because the answer is to IGNORE the
field: the client gets the whole representation, which every client
understands. 416 would assert that what was asked for does not exist,
which is not a claim to make about a field that was not understood. The
three real forms are covered on the accept side.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ac8f77210
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
THE SATB TAKE NO LONGER COPIES, which removes a hole I put there. Staging into a scratch buffer that had to be grown put a realloc on the take path, and when it failed the take kept only what fitted and left the rest queued, reasoning that the tail costs one more take. Review found the hole in that: the drain loop stops when a batch marks nothing new, and the regress that would pick the tail up is bounded by CN1_SATB_MAX_REOPENS, so a large enough tail is still queued when the sweep starts. The cap's safety argument does not cover those entries -- it rests on a reference stored after the fixpoint being already marked or fresh, while a retained DELETION-barrier entry names exactly the object that is neither, and reclaiming it is a use-after-free a collection later and nowhere near here. Swapping the two buffers is what this function's own comment always said it did. A swap cannot half-succeed, so the failure mode is gone rather than handled, and the memcpy goes with it. The log is then handed back a capacity worth having, because the two buffers alternate and a take would otherwise leave it holding the smaller one -- nothing at all, on the first take -- and make the next burst climb back through a doubling storm during a mark. That realloc is allowed to fail: the log is empty at that instant, so there is nothing to lose, which is exactly what was not true of the one removed. NOT FULLY SETTLED, and worth saying: GcOverflowSpiral timed out once in the parallel suite with the swap in, and did not reproduce in three more runs of the same set, against one clean run without it. That failure landed right after a packaged build on the same machine and the test documents itself as load sensitive, but one unexplained timeout in the collector is not something to record as noise. The capacity restore above addresses the only mechanism by which the swap could plausibly have caused it. CI runs this suite on three legs per push. SQLITE BIND COUNTS, and the reported reason for it was wrong in a way that matters. Too FEW parameters left the rest unbound, which SQLite reads as NULL, so an insert or update committed a row the caller never wrote. Review said the Java SE arm already threw; measured, it does not -- both arms accepted it, so this is a shared defect and both are fixed rather than one being brought into line with the other. The Java SE side also routed every parameterless call through a plain Statement, so a statement WITH placeholders and null params never reached the count check at all; that branch now takes only SQL with no '?' in it, which is what PRAGMA and the transaction verbs it exists for actually look like. Needed a new sqlite3_bind_parameter_count native, verified under CN1_NATIVE_VERIFY=strict. And a failed credential refresh is not an expired credential. ECS, EKS and IMDS are ordinary HTTP endpoints that can time out, and letting that out turned a blink into an outage of every S3 call up to five minutes before AWS would have stopped honouring what was already in hand. The cached credential is kept unless isExpiring(0) says it is genuinely gone. The fallback itself has no test -- reaching it needs a credential inside its margin AND a failing resolve(), with no seam for either -- so what is covered is the contract it rests on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cef2d0d04
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
COM_STMT_EXECUTE carries a null bitmap and a type table sized by the
CLIENT, while the server decodes them using the statement's own parameter
count. MySql read the prepared count and never compared it, so a mismatch
did not reliably produce an error -- it produced a different reading of
the same bytes.
MEASURED against a real MySQL server rather than reasoned about, because
the engines differ and only one of the four cases actually bites: with one
or more placeholders MySQL does catch a wrong count, but a statement with
NO placeholders and a value supplied simply EXECUTED. The check inserted
two rows where one belonged, with nothing said. That is the case review
named in its parenthetical and it is the one that was real.
The check lives inside the try so COM_STMT_CLOSE still runs: a refused
statement must not also leak one on the server.
Covered in DbCheck rather than SelfTest, because the three engines get
this wrong in three different ways and only a shared body of checks holds
them to one answer -- which is what that harness exists for. Measured on
all three, before and after:
SQLite committed the row with NULLs (fixed in the previous commit)
MySQL committed the row, no error (fixed here)
PostgreSQL refused it server-side (already correct)
Now 28/28 SQLite, 30/30 PostgreSQL and 30/30 MySQL, on the Java SE arm and
on the packaged binary against the same two live servers. A/B'd against
the live server: with the comparison removed the no-placeholder case is
accepted again and the row count goes back to 2.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65c112c09c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ding An outbound response was accumulated whole with no practical bound. The 2GB ceiling in the native is an integer-safety bound and nothing else -- it stops a length narrowing to a negative array size -- and the Java SE arm had no ceiling at all, so an endless or merely huge upstream reply grew memory until the container was OOM-killed. The low-speed timeout does not help when the bytes are arriving quickly. CN1_WEB_MAX_RESPONSE_MB now bounds it, defaulting to the same 64MB CN1_HTTP_MAX_UPLOAD_MB uses inbound. Both arms read the same variable with the same default and refuse the same response; a value outside 1..2047 plain digits is IGNORED rather than clamped, so a misconfigured bound does not silently become a different one than the caller set. Covered against a LOCAL server serving one byte past the bound, so the check needs no network and no patience, and A/B'd on both arms: with the comparison removed each accepts 1048577 bytes. Separately, start() bound the listener before anything looked at workerCount, and the two arms then failed differently. Java SE's executor threw -- "maximumPoolSize must be positive", measured -- but only after the listener and reactor were open, so the port stayed bound: the A/B run died on "Could not bind 127.0.0.1:53103" when it tried to use it again. The packaged pool created no workers at all and returned a server that accepts connections and queues them forever, which is worse than either. One check before the bind and neither can happen. The test for that deliberately does NOT rebind a released port to prove the listener was not leaked. The first version did, it raced the other test forks, and it failed in the very suite it was added to -- a flake I would have introduced to catch someone else's bug. The message separates all three outcomes on its own: an IOException naming workerCount, the executor's own wording, or a server coming back at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 204cd92be7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
stop() drained on inFlightRequests and http2Turns, and both of those are still ZERO while a task sits in the pool's queue or a worker is handshaking or parsing a request line. So a stop could decide nothing was running, free the TLS and HTTP/2 sessions and drop descriptors, and then let a task that ExecutorService.shutdown() still permits run straight into freed native state -- or onto a descriptor number reused by the next server, which releaseVirtualThreadSlot() exists to allow. pendingWork covers the queued window and activeRequests the worker's whole stay on a connection. Both already existed; neither was consulted here. All four now live behind one workOutstanding(), used by both drain loops AND the guard that decides whether the sessions are safe to free -- the comment on the first loop records http2Turns being added to one of them and not the others, which is this same drift once already, and one predicate is what stops a third time. And the pollers are closed. Each Reactor owns an epoll or kqueue descriptor, a Selector on Java SE, and nothing ever closed them: a process that stops and starts a server leaked one per cycle. Host 0 SHARES the main reactor, so closing every host's poller and then the reactor would double-free that one rather than release two. Closed only on the clean path, and only after joining the poll loops: the early return leaves workers running and they reach the reactor, so closing it under one is the use-after-free the session sweeps already refuse to risk -- the same trade, a descriptor in a process that is about to exit. NO NEW TEST FOR THE DRAIN, deliberately. The window is between a worker being handed a descriptor and entering the handler, and every way I could force it open depends on thread timing -- I added a racy test one commit ago and it failed in the suite it was added to, so I am not adding another. What can be said: the change only ever makes stop() wait LONGER, never less, and the poller close is exercised by every stop() in the suites, which is where a double close or a close under a live loop would surface. Both arms stay at 172/172. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27b3f2dcb8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| curl_slist_free_all(headers); | ||
| return 0; | ||
| } | ||
| curl_easy_setopt(curl, CURLOPT_URL, urlCopy); |
There was a problem hiding this comment.
When an application passes a caller-controlled URL to Web.request(), the packaged runtime hands it directly to libcurl with every compiled-in protocol enabled; both shipped curl configurations include file://, so a request such as file:///etc/passwd reads a local file into the handler-visible response body, and protocols such as FTP may also be available. The class documents an HTTP/HTTPS client, and the Java SE implementation only accepts HttpURLConnection, so explicitly allow only HTTP and HTTPS for both initial requests and redirects.
Useful? React with 👍 / 👎.
| static size_t cn1WebHeader(void* contents, size_t size, size_t count, void* userp) { | ||
| CN1WebResponse* r = (CN1WebResponse*)userp; | ||
| size_t total = size * count; | ||
| char* grown = (char*)realloc(r->headers, r->headerLength + total + 1); |
There was a problem hiding this comment.
When a hostile or compromised upstream emits many legal header lines, this callback keeps reallocating the aggregate header block without any ceiling; libcurl's per-line limit does not bound the number of lines, so a fast header stream can exhaust the backend process before a body arrives. Fresh evidence in the current revision is that CN1_WEB_MAX_RESPONSE_MB is enforced only by cn1WebWrite(), while this header callback remains unbounded; apply a practical aggregate header limit here as well.
Useful? React with 👍 / 👎.
| // One cache line per host, so the stripes never share one. | ||
| server.servedStripes = new long[hostCount * SERVED_STRIPE_STRIDE]; | ||
| for(int iter = 0 ; iter < hostCount ; iter++) { | ||
| server.vtHosts[iter] = new VtHost(iter == 0 ? reactor : Reactor.create()); |
There was a problem hiding this comment.
In virtual-thread mode with more than one host, if creating any additional Reactor fails—for example under descriptor exhaustion—the exception escapes after the listener is bound and VT_SLOT_TAKEN has been claimed. No HttpServer is returned, so the caller cannot close the listener or earlier reactors, and subsequent startup attempts either fail on the retained port or permanently fall back from virtual threads; construct these pollers under cleanup that closes all prior resources and releases the slot on failure.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a server-side runtime that runs a Codename One handler through the ParparVM
pipeline: Java or Kotlin translated to C and compiled into one static native
executable with no JVM under it. About 8 MB, a few milliseconds to first
connection, about 3 MB idle.
What this is for, and what it is not
It does not replace Spring Boot, Jakarta EE, Quarkus or Micronaut, and it is not
trying to. Those carry a container, an ORM, a security stack and twenty years of
operations; none of that is here or planned.
It targets the region where the JVM's assumptions stop paying: cold starts
charged per invocation, baseline memory charged for an instance's life, sidecars,
edge locations, short-lived processes. That is where Java is thin and Go and
Node dominate, and where a Java shop ends up carrying a second language and a
second copy of every model that crosses the boundary. Either as a piece of a
larger deployment or as the whole server for a small project.
The vertical integration is the other half: one
@RestClientinterface generatesthe app's asynchronous client and the backend's synchronous half plus its
dispatcher, so a contract change is a compile error rather than a response the
app fails to parse in the field.
Where it stands against Go
vm/backend/benchmarksholds the harness. Two pinned cores, 64 connections,interleaved with rotating arm order, against fasthttp:
The /json figures are the generated-DTO path answering off a pooled response.
A handler that returns a
LinkedHashMapper request is about 0.58x, which thebenchmark keeps as its default because that is the honest cost of that shape.
Notable changes outside vm/backend
cn1_globals.mgainscn1SatbTrim. The SATB write-barrier log and its stagingbuffer only ever doubled and were never given back, so a process that saw one
busy period kept the peak for life -- 8 MB of a 12 MB plaintext process was an
empty buffer. Trimmed in the sweep against the recent high-water mark. This
reaches every Codename One target, not just the backend.
maven/pom.xmlbuildsmaven/backend, which was in no<modules>block, sonothing built the artifact
BackendPackageMojoresolves at run time.cn1:backendandcn1:backend-package, and the@RestClientserver-half processor.
backendmodule, behind-Dcodename1.platform=backendso a client-only app pays nothing for it.Testing
BackendHttpIntegrationTest21/21, plus the database and JavaSE-runtime suites.GcHeapIntegrity,GcOverflowSpiral,GcUncooperativeThread,LargeArrayGc,BibopPageFloor.GcSteadyState's 768 MB ceiling scenario fails on the dev machine and failsidentically with the SATB change stashed (895.8s against 913.7s, same timeout,
same scenario), so it is the known local failure rather than a regression. It
is
@Tag("benchmark")and runs in the benchmark job.--failure-level WARN,structure, cross-references, snippets, links, paragraph capitalization.
codenameone-maven-plugin: 0 findings. Copyright, controlcharacters and cast-semantics gates clean over the branch.
backend module compiled against
codenameone-backend.PMD and Checkstyle were not run locally; CI is the first run for those.