Server-side backend: a native, JVM-free runtime for Codename One handlers - #5741
Open
shai-almog wants to merge 305 commits into
Open
Server-side backend: a native, JVM-free runtime for Codename One handlers#5741shai-almog wants to merge 305 commits into
shai-almog wants to merge 305 commits into
Conversation
gcSatbCap only ever doubled. Nothing shrank it, and the take-side staging
buffer inside cn1SatbTake grew the same way, so a process that saw one busy
period kept both at the peak for its whole life. Measured on the backend, with
gcSatbTop read as 0 every time it was sampled -- none of it was in use:
plaintext satbCap 8MB of a 12MB RSS
/json DTO route satbCap 16MB
/json map route satbCap 8MB
Two thirds of the plaintext process was an empty write-barrier log. It is also
what made the footprint look unrelated to anything else: the run with the FEWEST
BiBOP pages had the MOST resident memory, because it was the one whose barrier
traffic had reached 16MB.
Trimmed in the sweep, beside the page trim, against the high-water batch since
the last trim rather than the instantaneous depth -- which is 0 there by
construction and would shrink to the floor every cycle and re-grow through
several reallocs on the next burst. The 4x slack and doubling target mean a
steady workload settles at a size it keeps.
Only shrinks when the log is idle: a non-empty log is live data the mark phase
has not taken yet. A failed realloc keeps the existing buffer, because realloc
is not required to succeed just because the block is getting smaller.
Interleaved with rotating arm order behind a calibration gate, n=3:
/json RSS 57.7MB -> 24.1MB 590056 rps against 587744
/plaintext RSS unchanged 647380 rps against 646246
Throughput is unchanged on both routes; the memory is 58% off the allocating
one. Plaintext does not move because response pooling runs one collection per
15s, so the trim almost never fires there.
GC gate: GcHeapIntegrity, GcOverflowSpiral, GcUncooperativeThread, LargeArrayGc,
BibopPageFloor and the 21 BackendHttpIntegrationTest cases all pass.
GcSteadyState's 768MB-ceiling scenario fails, and fails IDENTICALLY with this
change stashed (895.8s against 913.7s, same 600s timeout, same scenario) -- it
is the known local failure on this 16-core machine, not a regression.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The deferred-JSON path already avoided every copy on the body side: Json.write
serialises straight into the connection's reusable ByteSink, so no byte[] and no
String is ever materialised for the body. What it did not avoid was the Response
itself. Response.jsonValue is a static that allocates one per call, and that was
the ONLY thing the route allocated.
Profiled with -DCN1_GC_CONFORM over 10.9M requests on the DTO route:
88.1 bytes/request, and the histogram has one row that matters --
com.codename1.backend.HttpServer.Response bytes=961269408 count=10923516
count is one per request. The plaintext route had been pooled already and sat at
0.1 bytes/request. Request.respondJson puts the JSON route on the same footing:
the connection's pooled Response, reset and re-pointed at the value.
That takes the route to 0.1 bytes/request, and with nothing to collect the
collector stops running: ZERO cycles in a 15s run against 205. Which is the
whole point -- the collector shares the server's cores, so on this machine a
route that allocates pays for it in its tail, not in its allocator.
Production build, interleaved with rotating arm order, n=3:
rps p50 p99 cpu/req RSS gc cycles
pooled (3) 672158 72us 178us 2.33us 10.6MB 0
unpooled (2) 600949 89us 2843us 2.59us 22-27MB 205
fasthttp 592709 96us 1347us 2.51us 12.3MB -
1.134x fasthttp's throughput, 7.6x its p99, less cpu per request than it spends,
and a smaller resident set. For reference fasthttp is not allocation-free here
either: GODEBUG=gctrace=1 over 11.4M requests shows 60 collections, 3->3->0MB
each, about 16 bytes per request.
Mode 2 stays exactly as it was so the cost of the Response remains measurable
against mode 3, and mode 0 stays the default -- it is still the honest cost of a
handler that hands back a Map.
reset() already clears deferredJson and hasDeferredJson, so a pooled Response
reused for a plain body cannot carry a stale value into the next response.
BackendHttpIntegrationTest 21/21 and BackendDatabaseTest pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
scripts/check-copyright-headers.sh --base master reported it as the one file in the branch without a header. Same Codename One GPLv2 + Classpath Exception header as the demo beside it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
maven/backend/pom.xml was complete -- parent codenameone, sources from
${backend.dir}, a parparvm-sources classifier beside the compiled jar -- and
listed in no <modules> block, so nothing built it. The only reason it resolved
here was an install run by hand into a per-checkout repository months ago.
That matters because BackendPackageMojo resolves the artifact at run time:
resolve("com.codenameone", "codenameone-backend", ...)
so on a fresh clone, in CI, and in a release, cn1:backend-package and cn1:backend
would fail to find a runtime that the build never produced.
Placed after sqlite-jdbc, its only dependency. Verified by deleting the
hand-installed copy first and building the module from the reactor: all four
artifacts come out, including the parparvm-sources classifier the package goal
needs.
Note for the release: this now publishes codenameone-backend alongside the other
modules, which is the point -- an app cannot depend on a runtime that is not
published.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The branch adds a server-side runtime, two Maven goals and a published artifact, and the guide said nothing about any of it. This is the chapter a reader needs before deciding whether the thing is for them. It leads with what the backend does NOT replace, because that is the question a Java developer asks first and the wrong answer is expensive. Spring Boot, Jakarta EE, Quarkus and Micronaut carry a container, an ORM, a security stack and twenty years of operations; none of that is here and none is planned, and a team running a Spring service that works should keep running it. What the chapter argues instead is where the JVM's assumptions stop paying: cold starts charged per invocation, baseline memory charged for the life of an instance, sidecars and edge locations and short-lived processes. That is the region where Java is thin on the ground 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. The backend exists to remove that reason to leave Java, not to compete where the JVM already wins. The vertical-integration section is the other half of the argument: one annotated interface generates the app's asynchronous client and the server'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. Also covers the two goals and how they differ, the four packaging targets and why both libcs exist, the Lambda custom runtime, and a limits section that names the library ecosystem, the absent framework structure and the fact that throughput alone is not a reason to move. Placed in a new "Server side" part. Figures quoted are the ones measured here: about 8 MB static, a few milliseconds to first connection, about 3 MB idle. Gates: vale 0 issues across the guide, asciidoctor lint clean at --failure-level WARN, structure 123 documents, cross-references 1699 anchors, paragraph capitalization clean, snippets and links unchanged. The three vale-skip comments are documented exceptions where the contraction rule would attach a verb to the wrong word. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A generated project had no server side, so a developer who wanted one had to work out the module layout, the dependency and the two goals from the guide and write the pom by hand. The archetype now emits a backend module beside the platform ones. It is NOT built by default. The module sits in a profile activated by -Dcodename1.platform=backend, exactly like the platform modules, so a client-only app pays nothing for it and asking for it is explicit. The generated module deliberately does not depend on codenameone-core. A server has no display, and the compiler saying so at the import is more useful than a crash at start-up; the pom comment says this and points at the two ways to share types with the app, a module both depend on or a @restclient contract. BackendServer is a working handler rather than a stub: it installs the shutdown handler so a container stop drains in-flight requests, reads PORT and WORKERS from the environment, answers /healthz, and blocks in awaitTermination because the reactor threads are detached and a returning main would exit silently. Verified end to end rather than by inspection: installed the archetype, generated com.example.demo:mydemo from it, and compiled the generated backend module against codenameone-backend. The Java lands at backend/src/main/java/com/example/demo/BackendServer.java with the package substituted, the pom resolves to mydemo-backend with the right mainClass, and the root pom carries the profile. The first attempt failed generation outright: an XML comment cannot contain "--", and the wording had one. Worth knowing because the archetype reports it as an XmlPullParserException against a position in the generated file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A download had no server side, so the initializr answered the client half of a project and left the other half to be assembled by hand from the guide. The skeleton in common.zip now carries a backend module beside the platform ones, with a working handler rather than an empty directory. It is not built by default. The module sits in a profile activated by -Dcodename1.platform=backend, the same shape the platform modules use, so a client-only download pays nothing for it. GeneratorModel validates the new module's coordinates like the others but deliberately does NOT require a dependency on the generated common module -- and the test asserts that dependency is ABSENT. common is compiled against codenameone-core, a server has no display, and requiring it here would enforce exactly the mistake the module's own comment warns against. Verified against the real zip by mirroring what the generator does: apply the same content and path substitutions, normalize whitespace the way normalizedPom does, then assert the fragment validateModulePomCoordinates looks for. The coordinates resolve to <pkg>:<app>-backend:1.0-SNAPSHOT, the handler lands at backend/src/main/java/<pkg>/BackendServer.java with its package rewritten, and the root pom keeps balanced profile tags. The initializr's own suite could not be used for this: every test class in that module reports "Tests run: 0" locally, pre-existing and not specific to these sources, so the assertions added here get their first real run in CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
scripts/check-copyright-headers.sh --base origin/master reported it: template sources under archetype-resources are tracked files, so the gate covers them, and the sibling stub in javase/src/desktop/java carries the same header. The initializr's copy of this handler deliberately does NOT get one: its template sources live inside common.zip, are not tracked as source, and none of them carry a header -- MyAppName.java in barebones-src.zip opens on its package line. Each path keeps its own convention rather than inventing a third. Re-verified after the edit: the archetype still generates, the package line lands at 23 under the header, and the generated backend module still compiles. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
scaffolding-integrity failed on the first CI run: the validator holds a hardcoded set of the POMs it expects inside common.zip, and the new backend module is not in it. Validated like a platform module, with one check inverted. The platform modules must depend on the generated common module; the backend module must NOT, because common is compiled against codenameone-core and a server has no display. So the common-dependency assertion is turned off for it and replaced by its opposite -- finding that dependency there is now a failure rather than something the script is silent about. This is the third place that had to learn about the module, after GeneratorModel's runtime guard and the matrix test. All three now agree on the same rule. Verified by running the whole scaffolding-integrity job locally: embedded coordinates, generated repositories, and the archetype/initializr settings parity check (39 hint entries in sync). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
![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: 751b6c25a3
ℹ️ 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".
Contributor
|
Developer Guide build artifacts are available for download from this workflow run:
Developer Guide quality checks: |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Collaborator
Author
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
Contributor
Cloudflare Preview
|
Collaborator
Author
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 166 screenshots: 166 matched. |
Collaborator
Author
|
Compared 166 screenshots: 166 matched. |
Collaborator
Author
|
Compared 166 screenshots: 166 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 181 screenshots: 181 matched. |
The first version argued for the backend and taught nothing: no project layout, no code, no architecture, no numbers, and nothing about the concurrency model that makes the thing work. Rewritten around what a reader actually needs. Added: * Two diagrams. One shows the build pipeline and the request path -- Java to bytecode to C to one binary, then connection to host thread to virtual thread to handler. The other is a slope chart of p50 against p99 on a log scale, because the tail is the interesting part and a table hides it. * A first server: the generated module layout, the handler the archetype emits, and what the three non-obvious lines in it do (shutdown draining, answering off the pooled Response, awaitTermination). * Virtual threads and the request loop: one virtual thread per connection rather than a pooled worker per request, one host thread per core, descriptor affinity as the reason the scheduler needs no locks, and the measured 16-hosts-versus-2 result that fixed the host count to cores. * A database section, and a fullstack section showing one @restclient interface generating the app's async client and the server's sync half plus dispatcher. * Measured numbers instead of adjectives, from the harness in the repository. The measurements, two pinned cores and 64 connections, medians of three interleaved runs: req/s p50 p99 cold start resident native musl 595,610 0.090 ms 0.249 ms 0.77 ms 10-40 MB native glibc 547,761 0.065 ms 4.06 ms 2.88 ms 14 MB Go fasthttp 496,293 0.104 ms 2.63 ms 2.39 ms 6.3 MB same code, JVM 187,745 0.260 ms 1.60 ms 82.5 ms 190 MB The JVM row is the same handler and the same protocol source, run through impl/javase instead of impl/parparvm, so the difference is the runtime underneath and nothing else. Cold start is the column that matters: under a millisecond against 82.5, which is the whole serverless argument. The chapter also says plainly that the tail follows allocation rate rather than the runtime badge -- the flat musl line is a route that allocates 0.1 bytes per request, and the same server allocating a map per request has an 80 ms tail. GraalVM is named as absent and why: this handler cannot run on it, since the runtime's natives are ParparVM's, so a comparison would be a different server against a different framework reported as a toolchain result. Gates: vale 0 across the guide, LanguageTool 0 (status=ok, run against the rendered HTML on JDK 17), asciidoctor clean at --failure-level WARN, structure, snippets, code blocks and unused images all pass. epoll and microbenchmark added to the LanguageTool accept list as real terms its dictionary lacks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CodeQL flagged this on the PR: the jar unpack built each destination straight from the entry name, so an entry called "../../../../etc/whatever" resolves outside the directory being unpacked into and the copy writes wherever the entry says. That is Zip Slip, and here it runs with the developer's privileges during an ordinary package against whatever artifact the coordinates resolved to. Both branches now go through resolveInside, which canonicalises the destination and refuses anything that does not land under the root. Canonical rather than textual because ".." is not the only way out -- a symlinked parent resolves elsewhere too and passes a string check -- and the separator is appended to the root so a sibling whose name merely starts with it cannot satisfy the prefix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d943d5e4fd
ℹ️ 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".
Collaborator
Author
|
Compared 160 screenshots: 160 matched. Benchmark Results
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
Seven findings from the PR review, each verified against the code before being acted on. HttpServer, two request-smuggling boundaries: * Whitespace before a header colon was trimmed away, so "Content-Length : 5" became a valid Content-Length here while an intermediary in front rejects that line or reads it as a different field. RFC 9112 5.1 says a server MUST reject it, which is what the obsolete-folding check beside it already does. * Transfer-Encoding was decided per field with a substring test, so a second Transfer-Encoding overwrote the first and an unsupported coding was ignored entirely -- either one leaves the body to be read as the next request. Every instance is now joined and the list must end in chunked, with anything else refused rather than framed on a guess. StaticFiles: a configured prefix of /assets matched /assets2/logo.png and stripped it to /2/logo.png, serving from the document root a URL outside the namespace the handler was mounted on. The prefix now has to end on a segment boundary. Json: the String-returning writer left Short and Byte out of its numeric branch while the ByteSink writer had them, so the same value was 1 through one API and "1" through the other. RestServerAnnotationProcessor, three: * A Set-typed @Body was bound by casting bodyAsList's ArrayList to Set. That is the cast the file's own comment warns about -- the JVM throws before the handler runs and the translated target does not check at all. It converts now. * Percent escapes were appended one character per octet, so %C3%A9 arrived as two characters instead of one accented letter. Consecutive escapes are gathered and decoded as UTF-8. * Float, Short and Byte DTO fields fell through to guardedCast, which returns null because the JSON reader only ever produces Long or Double, so valid client values were dropped silently. BackendPackageMojo: the translator received only JavaAPI and this goal's own output, never the module's dependencies. A backend using a type from a shared contract module compiled -- javac had it on the classpath -- and then failed to translate, which is the DTO-sharing arrangement the generated project recommends. BackendJavaSeRuntimeTest: skip rather than fail when the local repository has no codenameone-core for generate-contract.sh to build against. vm-tests does not install it, so the failure was an environment gap; this routes it through the same skipOrFail the database tests use, and CN1_BACKEND_REQUIRED still turns it back into a failure where the backend is meant to run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PostgreSQL builds its SCRAM verifier from the SASLprepped password. There is no Normalizer here -- this class is translated, so it has only what vm/JavaAPI and CLDC11 define -- and the comment at the PBKDF2 call said such a password "is rejected" while nothing rejected it. A no-break space pasted out of a browser therefore failed as "password authentication failed", which sends the reader to look at the role's grants. Rejecting every non-ASCII password would be the wrong reading of the same limitation: SASLprep leaves an NFKC-stable character alone, so an o-umlaut password matches the raw bytes sent here and refusing it would break accounts that authenticate today; and where SASLprep FAILS rather than maps -- the prohibited tables -- PostgreSQL falls back to the raw password, which is again what goes out. Only the narrow middle breaks, and its two mapping tables are small and closed, so B.1 and C.1.2 are matched by value and refused with a message that names the configuration rather than the credentials. A character NFKC would rewrite for another reason is still undetectable here, and the comment now says so instead of claiming the rejection. Two findings in the same round were refused, each answered in the code because a PR thread is not read by the next reviewer: A raw control byte in the request target is already refused -- targetDecodesToUtf8 opens with that rule, before any decoding, for both protocols, and the suite asserts it. What was true is that it then reported "not valid UTF-8", which describes neither a TAB nor the reason it was refused; both protocols now name both causes. A NUL in a MySQL connection field is already refused inside writeCString, which is the one place user, database and plugin are all written. This is the second review to ask for it at the call site, so the call site now says where it is. 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: ef30bbd16a
ℹ️ 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".
…sence fromEnvironment() answered null whenever either AWS_ACCESS_KEY_ID or AWS_SECRET_ACCESS_KEY was missing, so resolve() went on to the container endpoint and then to instance metadata. A misspelled key in a Kubernetes Secret, or one that failed to mount, therefore did not fail the workload: it ran under the node's role instead, with whatever that role can do, and the only evidence was an access denied somewhere else -- or none at all, where the node role happened to be the wider one. Both absent is still absence, which is how a deployment that means to use the instance role says so. One present is refused, naming the two variables and nothing else: the half that IS set is a live credential and this message goes wherever the caller logs it. Every other AWS SDK draws the line in the same place; botocore has a name for it, PartialCredentialsError. The decision moves into credentialsFrom(id, secret, token) so it can be driven with values, since a process cannot set its own environment -- the same reason CredentialEndpointProbe exists, and it grows one more door rather than the runtime growing public API for a test. The session token stays optional; it is not part of the pair. 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: a7f7bfe389
ℹ️ 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".
Three review findings, one of which turned out to be the opposite of what it
described.
Http.readResponse read to end of stream before looking at any framing, so a
peer that answers completely and keeps the connection open -- one that speaks
keep-alive whatever was asked, or a control endpoint that never hangs up --
left this blocked in read() with the whole response already in the buffer.
There is no read deadline on the socket, so that is for as long as the process
lives: in LambdaRuntime it strands the invocation loop for the life of the
instance. It now stops where Content-Length, the last chunk, or a bodiless
status says the message ends, and waits for end of stream only when nothing
else frames it. A deadline was the other suggestion and would have been wrong:
GET /invocation/next is a long poll that is SUPPOSED to block until work
arrives. What is still unbounded is a peer that stalls mid-message, which needs
a socket deadline this runtime has no native for, and the comment says so.
Reactor.modify updated the selector's interest and left the one-shot
bookkeeping to whatever add() had last said. The finding named the Java SE arm
and said the translated ones take the flag from every call; measuring says
kqueue does not. EV_DISPATCH is latched at creation there -- re-adding an
existing knote re-enables it but neither sets nor clears the flag -- so a
descriptor added ONESHOT and modified to level still fired once and stopped,
and one added level and modified to ONESHOT kept firing. The C said as much in
a comment: "kevent's ADD is idempotent, so a modify is the same call". Only
epoll was right, because EPOLL_CTL_MOD replaces the whole mask. Fixing the Java
SE half alone would have made two arms disagree instead of one, so kqueue now
drops the filter before re-adding it and both halves of the rule are checked,
in both directions, on every arm.
RestControllerAnnotationProcessor checked a route pattern for ASCII only as far
as byteArrayLiteral saw it, and for a dynamic route that stops at the first
'{'. Everything after a variable was emitted as a Java string and compared
against widened wire octets, so /files/{id}/cafe-with-an-acute matched neither
the raw spelling nor the percent-encoded one: it packaged cleanly and answered
404 forever. The whole pattern is now refused at build time, as the contract
generator already does, and in the same words.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f11465ed1
ℹ️ 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".
Both findings are on last round's code. chunkedEnd added the peer's chunk size to the position of the chunk's first byte in an int. parseChunkSize refuses only what overflows its own accumulator, so "7fffffff" is a legal thing to receive, and after + chunk + 2 then wrapped negative -- which read as "the chunk has not arrived yet" and put a negative cursor back for the next pass. Measured with the fix removed: the walk indexes the buffer at -2147483590 and the caller is handed a failure whose entire message is that number. The review reading the same code predicted an endless re-parse of the one size line, which is what it would be if indexOfCrLf did not clamp its start; the code comment says both, because the arithmetic is what is wrong either way. The Content-Length path had the same shape -- bodyStart + declared could wrap and leave total negative, silently throwing away the framing the loop had just worked out -- so both compute in a long now. The kqueue modify added last round deleted only the filters the NEW event set named, so changing READ to WRITE left EVFILT_READ armed: await() went on reporting the descriptor readable for an interest the caller had replaced, and since kevent reports one event per filter the same descriptor arrived twice in one await. A modify replaces an interest -- EPOLL_CTL_MOD does it with its mask and interestOps() does it on a Selector -- so both filters are dropped before the requested ones are added. 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: 5e17aa5326
ℹ️ 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 file has begun at its package statement since it was added, and the copyright gate is diff scoped, so nothing ever put it in front of the check. Touching it for any reason does -- which is what happened on master, where an unrelated developer-guide commit added snippets to it and the gate then failed on a header that had been missing all along. This PR inherits that failure because CI builds the merge commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Redirects were followed freely whenever the caller supplied no headers, on the reasoning that there is then nothing to leak. A body is something to leak. A 307 and a 308 preserve the method AND the payload -- that is precisely what separates them from a 301 or 302, which libcurl and HttpURLConnection turn into a GET -- so a POST redirected off-domain arrives at the new host complete: a form with a password in it, a token exchange, a signed document. libcurl drops Authorization when the host changes and has never done the equivalent for a body, because nothing can know what is in one. A bodied request is also the state-changing kind, so following one blindly can perform it twice, the second time somewhere the caller never named. Measured, not argued: with the old condition in place, a POST carrying "s3cret-payload" and no headers is answered 307 by one listener and the payload arrives at a second one named by a different hostname. The check reports "200 the body arrived" there and "307 nothing" with the rule in place. The finding named the packaged arm. The Java SE twin gated setInstanceFollowRedirects on the same condition, so both are changed; fixing one would have left the arms disagreeing about which requests are safe to follow, which is the failure mode that comment pair exists to prevent. The control is a bodiless GET through a 302, which both arms still follow. 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: 253a8423de
ℹ️ 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".
LanguageTool's coherency rule fails the guide build on any match, and the guide held both variants: four -ise forms against twenty-three -ize ones. It flagged a single "recognized" and suggested "recognised", which would have satisfied it for one build and moved the complaint to the next American spelling, since the rule reports the first deviation rather than all of them. Unifying on the majority leaves it nothing to find. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… slot Two findings on the previous round's code. Redirects were followed freely for any request with no headers and no body, and an empty DELETE or PUT is exactly that. Nothing leaks there -- but leaking is half the problem: a 307 and a 308 preserve the METHOD, so following one repeats the state change at a Location the caller never chose, and an endpoint that has been taken over needs only to answer one redirect for the delete to happen on its host. GET and HEAD are the safe methods (RFC 9110 4.2.1) and are now the only ones followed freely; anything else, including a method spelled in lower case, is treated as unsafe, which is the direction to be wrong in. Measured with the rule removed: an empty DELETE and an empty PUT both arrive at the second listener. kqueue registers a filter at a time and reports one event per FILTER, so a descriptor watched for READ and WRITE that becomes both at once was copied into readyFds twice, while epoll reports it once with a combined mask and a Selector answers with one key. That is one connection handed to two workers, each believing it owns it -- and in a one-slot array the second event stays in the kqueue, so the descriptor returns on the next wait with no modify(), which is what a ONESHOT registration promises cannot happen. Coalesced by descriptor. The check for that one passed in both directions at first, because it registered the descriptor before the bytes it had just written arrived: only the WRITE filter was ready, so one event was the right answer for the wrong reason. It waits for readability now, and fails with "expected <1> but was <2>" when the coalescing is removed. Both arms again for the redirect rule: the Java SE twin gated setInstanceFollowRedirects on the same condition. 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: e6173d9ee8
ℹ️ 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".
…ling Three findings, all on paths this PR added. startTls read the descriptor under the monitor and then let go of it for the length of the handshake, which is as long as the peer takes. A close() during it found inFlight at zero and released the descriptor outright -- and the number is then free for the next accept() or connect() in the process, while OpenSSL's socket BIO holds nothing but that number, so the next handshake syscall reads or writes somebody else's connection. The recheck afterwards stops the session being published; it cannot unsend bytes. The handshake now takes the same claim read() and write() take, so close() shuts the descriptor down and leaves the number in place until release(). No test: the window is between a close and the kernel reusing the number, which this suite cannot force. That handshake also had no deadline, on either arm. connectTimeout is spent reaching the port, so a host that completes TCP and then stops talking parked the caller for good -- every handler opening a connection to a partly failed database doing the same, against a bounded pool. The obvious fix is the wrong one: SO_RCVTIMEO bounds ONE read and OpenSSL does several, which is the argument the inbound handshake already settled. So the loop that settled it is now shared rather than copied -- one flag picks SSL_connect over SSL_accept -- and the client calls it with a budget from CN1_TLS_HANDSHAKE_MS, default 15000, which the Java SE twin reads from the same name. That twin can only express a per-read timeout through SSLSocket, and its comment says so rather than implying the two are identical. Measured with the bound removed: a handshake with a peer that accepts and says nothing returns after 20005ms, which is exactly how long the stub chose to hold it. CN1_HTTP_MAX_CONNECTIONS accepted a negative, and the admission check applies the ceiling only when it is positive, so -1 read as "no ceiling" -- from a typo, where 0 says it deliberately. It goes through the clamp now, with zero as its floor. The clamp's own comment said it was package visible so the runtime self-test could check it, and nothing ever had; the probe exposes it and four checks cover it, including that zero still stands. 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: 791b868338
ℹ️ 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".
💡 Codex Review Enforce the response limit before growing the buffer
When a close-delimited or incomplete response fills the buffer exactly to ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…RL escape The read buffer doubled whenever it filled, and the count that enforces CN1_HTTP_MAX_RESPONSE_MB ran after it: a response that reached the limit had already bought an array of twice it, so refusing 64 MB of unwanted body cost a 128 MB allocation, and on a small heap an OutOfMemoryError rather than the IOException the setting promises. At a large configured limit the doubling overflows instead and the array size goes negative. Growth now stops one byte past the ceiling, which is exactly enough for the count to still see the byte that proves the response too long, and a buffer already there is refused in the same words rather than grown. That half is argued from the code, not measured: the refusal reads identically whether or not the buffer doubled first, and the size of the allocation behind it is not visible from the suite. What the new check does cover is the ceiling itself, which this client had no check for at all -- the existing one is the other client's. Database URLs decoded a bare, truncated or non-hex escape by appending the '%' as a literal, so "p%ZZ" and "p%2" became passwords holding a percent sign: the client authenticated with a value the URL does not contain, and the operator read a remote authentication failure rather than a typo in their own configuration. RFC 3986 2.1 leaves no other reading, and the index is all the message says about it, the component being a password as often as not. Measured: with the rule removed all three spellings answer "read the URL". The request target takes the opposite decision, deliberately -- browsers do send a bare '%' and a server refusing one would reject real traffic -- and both comments now say so, since the asymmetry is a decision rather than an oversight. The cases went into the check that already existed for the UTF-8 half of the same rule, which is what its name was already claiming to cover. 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: 4f5c713a38
ℹ️ 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".
kqueue takes one change per filter and unregisterImpl asked for both in a single call, with a comment saying an ENOENT for an absent filter was harmless. It is harmless in itself; what is not is that there was no eventlist for kevent() to report per-change errors into, so it returns at the first failure and never applies what follows. A descriptor registered for WRITE alone has no read filter to delete, so the read delete failed, the write delete was never reached, and remove() reported success while await() went on handing back a descriptor its caller had deregistered. This is the same trap the modify path documents a few lines up -- "each delete goes in its own call because a failing change stops kevent() from applying the rest of a batch" -- written there while this one still batched. Measured with the batch restored: the check registers WRITE only, removes it, and await() still answers 1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
💡 Codex ReviewCodenameOne/vm/backend/src/com/codename1/backend/StaticFiles.java Lines 155 to 157 in cff614e  Keep fallback containment atomic
When CodenameOne/vm/backend/src/com/codename1/backend/StaticFiles.java Lines 182 to 184 in cff614e Prevent directory redirects from becoming network-path URLs
When the root mount serves an existing directory requested with a target beginning ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…ct here Where openBeneath cannot settle containment -- the Java SE arm, and any kernel without openat2 -- the realPath check is a SECOND lookup, so it answers a question about the path while the response sends bytes from a descriptor nobody looked at again. An attacker who can write symlinks into the document root points one outside for the open and back inside for the resolve, and the file served is the one the check never saw. The comment there described that race and left it. The descriptor's inode is already in hand from the stat, so opening the resolved path and comparing binds the two: the file this response will send has to be the file that was proven contained, and a swap in either direction makes them differ and is refused. Two limits, both written down rather than implied: a hard link inside the root to a file outside it cannot be told apart -- and that file does have a name inside the document root -- and a platform that reports no identity at all keeps the older, weaker answer instead of refusing every request it cannot bind. A proven open pays for none of this. The directory redirect was built from the request's own path, leading run and all, so a target of "//name" answered "Location: //name/" -- which a browser reads as a scheme-relative URL whose AUTHORITY is "name". The tidy-up redirect of a static file server was an open redirect. It says an absolute-path reference on this origin now, with exactly one slash in front; a backslash counts, because the URL parsers browsers use treat it as a separator even though the path grammar does not. Measured with the old construction back: "//evil.example/". 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: 428a36734c
ℹ️ 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".
…ly buys Web.request(null, url, null, new byte[0]) went out as a POST. An empty array is not null, and the body copy allocates one byte even for none so that a failed allocation stays distinguishable from an empty one -- so the copy was non-null, POSTFIELDS was set, and libcurl switched to POST mode for a call that supplied no method at all. That means GET on the Java SE twin and in every reading of this API, so the same call reached a different route, or performed a state change, only once packaged. Gated on a positive length now, or on a verb the caller actually named, which leaves an explicit POST with an empty body exactly as it was. Measured with the gate removed: the listener sees POST. The second finding is right and its remedy is not available here: there is no fstat for a FileChannel in public Java, and a Java-side containment-preserving open is the same racy open-then-check openBeneath already declines to imitate. What was wrong is what the code CLAIMED. The comment added with the identity binding said a swap "makes the two differ and is refused"; openBeneathImpl has said for longer, and more carefully, that comparing st_dev/st_ino afterwards narrows the window without closing it, because the second lookup is racy in the same way. Both now say the same thing: the resolved path has no symlinks left, so the final component cannot be swapped unseen, and a directory component of it still can. And the identity is not the same number on both arms. The packaged one reads it from an fstat of the open descriptor, so it describes the bytes being served; this one reads it from the pathname, because that is all it can. It looks identical on both sides of the API and only one of them is about the descriptor, so the field says so where it is captured. 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: 0c619b01fb
ℹ️ 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".
listObjects stopped at the limit only while it was positive, so a max of zero or below followed every continuation token to the end of the bucket -- the opposite of what was asked. The contract above it is "lists up to max objects" and there is no unlimited mode to preserve, so zero returns an empty list without asking and a negative is refused. Measured with the guard removed, against an endpoint nothing listens on: both spellings reach the network. endpointFor's comment says the ISO partitions are deliberately not guessed at, because a wrong DNS suffix there would be a silent misdirection of the same kind the China fix addressed -- and then it fell through to the commercial suffix for them, which is that misdirection. forRegion refuses them now. forEndpoint is untouched: it takes a host from its caller, so somebody who can reach one of those networks still can, and only the path that INFERS a host is closed. The first version of that check called the rule directly and proved nothing -- removing the call from forRegion left it green. It goes through forRegion now, which works without credentials because the partition is settled before they are resolved, and fails with "not refused" when the call is taken out. The comment says so, because the weaker shape is the tempting one. 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: ae4af4c5b4
ℹ️ 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".
connectTimeout is spent by the time a query goes out. Past it a read on a blocking descriptor has no deadline of its own, so a peer that finishes the handshake and then stops replying holds the calling thread until the process ends -- a request worker, or a virtual thread's carrier -- and a handful of stalled connections is a server that answers nobody. I had written this gap down while bounding the TLS handshake and left it; the review found it on the database path, where the parked thread is most clearly somebody else's. Tcp.setReadTimeout on both arms: SO_RCVTIMEO and SO_SNDTIMEO through a new native on the packaged one -- set once after connecting and left there, which is what that option is for and how ServerSocket already uses it -- and setSoTimeout on Java SE, which expresses only the receive half and says so. Reached through a socketTimeout URL parameter beside connectTimeout, refusing a negative for the same reason that one does. DEFAULT NONE, deliberately. pgjdbc and MySQL's own client default this to zero because a legitimate query can outlast any number picked here, and aborting one is a worse failure than the hang it prevents. The mechanism exists; a deployment that knows its queries turns it on. The deadline survives a TLS upgrade. Java SE's startTls rebinds the socket, so one set before it would have been left on the plaintext socket nothing reads again -- abandoned at exactly the point the connection starts carrying something worth protecting. Carried in rebind rather than re-applied by each driver: the first version of this commit had a comment claiming the drivers did it, which is the version that quietly would not have. Measured with the deadline not applied: a peer that accepts and says nothing holds the caller for 20002ms of the 20000 the stub chose. With it, 1500. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.