paulk-asert commented on PR #2928:
URL: https://github.com/apache/groovy/pull/2928#issuecomment-5660273477
Initial AI read:
> **Verdict: two real bugs correctly fixed, with tests that genuinely fail
without the change. Merge after covering the third fix.**
>
> Verified locally: 1580 tests pass across `groovy.util.*` and
`*ScriptEngine*` (including `GroovyScriptEngineReloadingTest`), `lintGate`
green, clean trial merge.
>
> **The one thing to add before merge.** The PR makes four changes; the JIRA
describes three. The fourth — `createCompilationUnit` resolving through
`rc.getResourceConnection(...)` instead of the engine's own roots-based method
— **has no test**. I reverted that hunk alone and the entire suite stayed
green. It's a real bug (with a connector-only engine, `roots` is empty, so the
engine's method can only throw) and its failure mode is silent, because the
loop does `catch (ResourceException e) { /* ignore */ }`. Dependencies just
quietly don't get added to the compilation unit.
>
> I checked each new test by reverting the main source: two genuinely fail,
one is characterisation rather than regression, and the forward-slash one can't
fail on Unix — though `groovy-build-test.yml` does run `windows-latest` on
`pull_request`, so CI covers it.
>
> **Two claims that are wider than the code supports** — worth rewording,
not reverting:
>
> - **The leak.** On the paths this PR touches, the built-in connector
returns an *unconnected* `URLConnection` — `openConnection` verifies with a
first connection, then returns a fresh second one. `AbstractHttpServlet`
likewise ends in `return url.openConnection()`. Nothing holds a descriptor, so
nothing leaks. And `forceClose` doesn't release a descriptor, it *opens* one in
order to close it — the PR's own test asserts exactly that (`assert
tracking.inputStreamOpened`). So on the default path the change adds an
open/close per lookup. It's still right for third-party connectors that return
connected connections, which is the population the PR is aimed at.
> - **Windows.** Both `ResourceConnector`s Groovy ships already normalise
`\` → `/` before use, so no shipped code was broken. The change benefits
third-party connectors and removes the obligation to normalise — a good reason,
just not the stated one. Since the contract is now load-bearing,
`ResourceConnector.getResourceConnection` should say so in javadoc.
>
> **One thing the change depends on and gets right**, worth a code comment:
enabling `setResLoader()` on the `GroovyClassLoader`-parent path raises the
question of which `CompilerConfiguration` the lambda reads, since that
constructor inherits the *parent's*. It binds to the enclosing engine's,
because `GroovyClassLoader` has no `getConfig()` at all. I confirmed by probe.
A future `getConfig()` added to `GroovyClassLoader` would silently flip it.
--
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]
