[
https://issues.apache.org/jira/browse/GROOVY-12404?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18115024#comment-18115024
]
ASF GitHub Bot commented on GROOVY-12404:
-----------------------------------------
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.
> GroovyScriptEngine resource loader leaks URLConnections and ignores a custom
> ResourceConnector
> ----------------------------------------------------------------------------------------------
>
> Key: GROOVY-12404
> URL: https://issues.apache.org/jira/browse/GROOVY-12404
> Project: Groovy
> Issue Type: Bug
> Reporter: Daniel Sun
> Priority: Major
>
> {{groovy.util.GroovyScriptEngine}} resolves dependent sources through
> {{ResourceConnector.getResourceConnection}}, but several call sites keep only
> the URL and never close the {{URLConnection}}.
> {{URLConnection}} has no {{close()}} method. The engine already has
> {{forceClose}} for this (see GROOVY-7186) and uses it from
> {{loadScriptByName}}. The resource-loader path does not.
> {{ClassNodeResolver.findClassNode}} only closes the connection when the
> source is newer; the cache-hit path leaks. Long-running hosts that resolve
> many scripts can exhaust file descriptors.
> The same resource loader has two further gaps:
> # When the parent class loader is a {{GroovyClassLoader}},
> {{ScriptClassLoader}} never calls {{setResLoader()}}. {{new
> GroovyScriptEngine(rc, new GroovyClassLoader())}} then ignores {{rc}} for
> {{loadClass}} / {{Class.forName}}.
> # Packaged names are mapped with {{File.separatorChar}}. On Windows the
> resource name uses a backslash as the separator instead of
> {{com/example/Foo.groovy}}, which is what {{ClassNodeResolver}} and
> {{GroovyClassLoader}} already use.
> h3. How to reproduce
> h4. Custom ResourceConnector ignored
> {code:java}
> File dir = File.createTempDir()
> new File(dir, 'Helper.groovy').text = 'class Helper { def ping() { "pong" } }'
> ResourceConnector rc = { String name ->
> File f = new File(dir, name)
> if (!f.exists()) throw new ResourceException('missing ' + name)
> f.toURI().toURL().openConnection()
> }
> def engine = new GroovyScriptEngine(rc, new GroovyClassLoader())
> engine.groovyClassLoader.loadClass('Helper')
> {code}
> h4. Resource-loader connection not closed
> With {{Foo.groovy}} present, install a {{ResourceConnector}} whose
> {{URLConnection.getInputStream()}} records that it was called, then:
> {code:java}
> engine.groovyClassLoader.resourceLoader.loadGroovySource('Foo')
> {code}
> {{getInputStream()}} is never invoked, so {{forceClose}} did not run.
> h3. Expected result
> * Connections obtained only for their URL are closed with {{forceClose}}.
> * A custom {{ResourceConnector}} is used even when the parent is a
> {{GroovyClassLoader}}.
> * Resource names for packaged classes use a forward slash on every platform.
> h3. Actual result
> * The resource-loader {{URLConnection}} is left open.
> * {{loadClass('Helper')}} throws {{ClassNotFoundException}} even though
> {{Helper.groovy}} is visible to the connector.
> * On Windows, connectors that do not rewrite backslashes to slashes fail to
> find packaged scripts.
> h3. See also
> GROOVY-7186 addressed a similar leak in {{getResourceConnection}} and added
> {{forceClose}}. The resource-loader path and the cache-hit branch of
> {{findClassNode}} were not covered.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
