allthingssecurity commented on PR #27678: URL: https://github.com/apache/camel/pull/27678#issuecomment-6122394010
Thanks for the review. All points are addressed in one new commit, `94dfe0f70dce`, on the branch rebased onto current main (`c3829069cb72`). camel-wasm with `install`: 55 tests, 0 failures (was 47). **Upgrade guide.** Done. `camel-4x-upgrade-guide-4_23.adoc` is identical to main again, so the CAMEL-25456 entry is untouched. The entry is now in `camel-4x-upgrade-guide-4_24.adoc` and also says that from 4.24 the endpoint runs a new instance per exchange, so the 4.23 note about discarding the instance only applies to the `wasm` expression. The 4.24 file is still not on main, so this PR creates it with the standard header and the index line; whichever 4.24 PR merges second needs a rebase that keeps both entries. **Orphaned worker on the body stream.** Confirmed and fixed as you suggested. With `timeout > 0` the body is read into a `byte[]` on the calling thread and the module reads that copy; without a timeout it is streamed as before. Stream caching is on by default, so the shared stream was normally an in-memory `StreamCache`, but it was still used by two threads. Trade-offs, now in the docs: the body is on the heap during the run, and reading the body is not covered by the timeout. Tests (bounded waits, no sleep): only the calling thread reads the stream; with a stream that blocks in `read()` and ignores interrupts the blocked reader is the caller, not a producer thread; after a timed out run the stream sees no further read. Four of the new tests fail on the previous producer (the reader was `Camel (camel-16) thread #17 - Wasm`). **Body after a failure.** Javadoc (`WasmExitCodeException`, `failOnNonZeroExit`) and the component page (a new table) now say what the body holds: unchanged for a `String`, `byte[]` or file; a `StreamCache` has been read and Camel resets it for the error handler; a stream that is not cached is at its end. With a `timeout`, where the input was buffered anyway, a stream that is not a `StreamCache` is replaced by the bytes read from it, so error handling still sees the content. Tested for each case. **`maxOutputSize` default.** Kept at 0 in this round: it is the design agreed with Claus in CAMEL-25531, and a default cap would fail exchanges whose output is legitimately large. The option description and the Limits section now carry a warning that an unlimited output fills the heap of the JVM, with 16 MiB as the example value. We are happy to switch to a 16 MiB default if you and Claus prefer it. **Thread pool.** Sorry this was left open. No cap and no new option, for this reason: a pool thread is taken only while an exchange waits for it, so the pool holds one thread per exchange in the endpoint at the same time, plus timed out workers until their next interrupt check. With the buffered input, the standard streams of a run with a timeout are all in memory, so the case you describe (a worker blocked in a host call) cannot keep a thread. Evidence: `timeoutInterruptsASpinningModule`, the new `timeoutInterruptsAModuleSleepingInAHostCall` (an hour in `poll_oneoff`, the worker ends after the timeout) and the blocked stream test above. **`environmentHeaders` ignores case.** Added to the option description (and the regenerated metadata, catalog and endpoint DSL) and to the docs, with the `TENANT` / `tenant` example. **API removals.** Listed in the 4.24 entry. One correction: `WasmEndpoint.getFunctionName()` did not exist on main (the field had no getter); the entry instead says that the `functionName` path parameter is gone and that the third `WasmEndpoint` constructor argument is now the module. **`toD`.** A warning in the URI section: a dynamic URI lets message data choose the module, including an `http:` location, and each new URI loads and parses a module; use a static URI, or a Choice between static endpoints. **Nits.** `memoryHint` no longer has a catch: `Instance.memory()` returns null for a module without memory and does not throw (new test `trapOfAModuleWithoutMemory`). `doStop` shuts the pool down before `super.doStop()`. **The two points you could not verify** (read from the bytecode of the Endive 1.1.0 jars with `javap`): - No preopens: the `WasiPreview1` constructor allocates descriptors 0, 1 and 2 (`Descriptors$InStream`, two `Descriptors$OutStream`) and then one per entry of `WasiOptions.directories()`, which we never set. `WasiPreview1.pathOpen` returns `EBADF` when `Descriptors.get(dirFd)` is null and `ENOTDIR` when the descriptor is not a `Descriptors$Directory`, so no path can be opened. `sockAccept`, `sockRecv` and `sockSend` throw `WasmRuntimeException`; `sockShutdown` returns `EBADF` or `ENOTSOCK`. - Interrupt checks: `InterpreterMachine.checkInterruption()` (throws `WasmInterruptedException`) is called from `call`, `callWithRefs`, `CALL_INDIRECT`, `BR`, `BR_IF` and `BR_TABLE`. `WasiPreview1.pollOneoff` sleeps with `TimeUnit.sleep` and turns `InterruptedException` into a `WasmEngineException`; `randomGet` checks the flag for every 4096 byte chunk; `ByteArrayMemory.waitOn` (atomic wait) throws `WasmInterruptedException`. `fdRead` calls `InputStream.read` with no check, which is the call the buffered input takes out of the picture. **Rebase.** Done, onto `c3829069cb72`; the PR is mergeable and the wasm catalog files and endpoint DSL are regenerated. _Claude Code on behalf of allthingssecurity_ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
