[
https://issues.apache.org/jira/browse/GROOVY-12665?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18137682#comment-18137682
]
ASF GitHub Bot commented on GROOVY-12665:
-----------------------------------------
paulk-asert opened a new pull request, #3205:
URL: https://github.com/apache/groovy/pull/3205
This bundles eight PRs by @codeconsole that all touch closure wrappers,
memoize serialization and object input stream class resolution, so the
combined behaviour can be reviewed and merged as one consistent set. The
individual PRs interact (several change the same lines of `Memoize.java`
and `Closure.java`), which made them hard to judge one at a time.
Cherry-picked in this order, following the merge notes in each PR:
| JIRA | PR | Change |
|---|---|---|
| GROOVY-12665 | #3186 | `dehydrate()`/`rehydrate()` through curried,
writable, composed and trampolined closures |
| GROOVY-12673 | #3194 | wrappers report and take the delegate, resolve
strategy and `thisObject` of the closure they wrap |
| GROOVY-12669 | #3190 | `dehydrate()`/`rehydrate()` through memoized
closures, with a fresh cache |
| GROOVY-12674 | #3195 | `memoizeAtMost`, `memoizeAtLeast` and
`memoizeBetween` closures serialize |
| GROOVY-12677 | #3198 | a memoized closure that has cached `null`
serializes |
| GROOVY-12679 | #3200 | async wrappers forward delegate, resolve strategy
and parameter types |
| GROOVY-12676 | #3197 | `ObjectInputStream` with a class loader resolves
primitive classes |
| GROOVY-12681 | #3202 | `ObjectInputStream` with a class loader resolves
proxy interfaces through that loader |
Four cherry-picks needed manual resolution (#3190 against #3194 in
`Memoize.java`; #3195 against #3190 in `Closure.java`, `Memoize.java` and
`AbstractMemoizeTestCase`; the test move in #3198; the cycle test in #3200),
each resolved the way the PR's own merge note describes. Scott's commits are
kept as his, with his authorship.
Two commits follow on top:
- **GROOVY-12669: serialize the memoize cache factories as named classes,
not lambdas.**
#3190 passed `(Supplier & Serializable)` lambdas as cache factories, so a
memoized closure serialized with a `SerializedLambda` naming a
javac-generated method of `Closure`. Such a stream cannot be read by 6.0.0
or 5.1.3, and the generated name changes with the enclosing method's
signature, lambda order and compiler. The factories are now four private
static nested classes of `Closure` with fixed `serialVersionUID`s. A plain
`memoize()` stream written this way reads on 6.0.0 and 5.1.3 (verified
against the distributions), and streams written by those versions read
here, as before.
- **GROOVY-12673: document how wrapped closures share delegation settings.**
Javadoc only. A memoized or trampolined closure calls the very closure it
was given, as it always has, and now also forwards a delegate or resolve
strategy set on the wrapper to it; `clone()` gives an independent copy.
Keeping the sharing (rather than cloning in the constructor) preserves the
6.0.0 behaviour where a binding made on the original after wrapping is
seen by the wrapper.
## Is anything deliberate being undone?
No. All eight main-source files are byte-identical between the 6.0.0 tag and
master, and a probe of about sixty behaviours this set touches gives the same
results on 4.0.33, 5.0.8, 5.1.3 and 6.0.0. The gaps being closed are
long-standing (the trampoline constructor, for example, has passed the
wrapped closure's delegate as `thisObject` since 2010), not 5→6 regressions.
The set only extends the deliberate 6.0.0 work: the deserialization cycle
check (GROOVY-12109, GROOVY-12276, GROOVY-12339), which walks fields rather
than getters and so is unaffected by the new forwarding, and
resolve-without-initialization in `newObjectInputStream` (GROOVY-12268).
## Behaviour changes, for the record
- Wrappers report the wrapped closure's `thisObject` (was `null` or the
owner).
- Memoize, trampoline and async wrappers forward `delegate` and
`resolveStrategy` to the wrapped closure. Previously the setting landed in
a
field nothing read, so a wrapped closure could not be handed to a builder
or to `with()`/`tap()`.
- Async wrappers, and trampolined writable or composed closures, report the
wrapped closure's parameter types instead of a single `Object[]`.
- `rehydrate()`/`dehydrate()` on a wrapper return a wrapper over the copy
instead of a copy of the wrapper still calling the old closure.
- The sized and soft memoize variants, and cached `null` results, serialize;
softly referenced values are dropped from the stream.
- The anonymous `AsyncClosureUtils$1`/`$2` wrappers become named classes, so
a 6.0.0 stream of a `wrapAsync` closure does not read here (async/await is
incubating).
- `Memoize` gains two public overloads taking a `Supplier` of the cache;
`japicmp` sees only additions.
A `COMPATIBILITY.md` entry (Groovy 6.0.1 — closure wrappers report and take
the delegation settings of the closure they wrap) describes all of the
above, since the intent is to backport the set to 6.0.x: every change either
turns an exception into working behaviour or is confined to introspecting a
wrapper closure.
## Verification
- Targeted suites (`groovy.lang.*Closure*`, `groovy.*Closure*`,
`org.codehaus.groovy.runtime.memoize.*`, trampoline, async, serialization
cycle, `BinaryStreamsTest`, `NioExtensionsTest`, `@Memoized` transform):
2837 tests, 0 failures, 7 skipped on the merged set; 207/0 after the
factory-class change.
- `./gradlew lintGate` passes.
- Cross-version serialization checked by writing streams with this build and
reading them with the 6.0.0 and 5.1.3 distributions, and the reverse.
## Not included
GROOVY-12661 (#3182) and GROOVY-12666 (#3187), which rehydrate packed
closures, are left for a separate review: they are the closure-packing
family and conflict with each other.
Closes #3186, closes #3194, closes #3190, closes #3195, closes #3198,
closes #3200, closes #3197, closes #3202.
> rehydrate() on a curried, writable, composed or trampolined closure throws
> ClassCastException or ignores the new owner and delegate
> -----------------------------------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12665
> URL: https://issues.apache.org/jira/browse/GROOVY-12665
> Project: Groovy
> Issue Type: Bug
> Affects Versions: 4.0.33, 5.0.8, 5.1.3, 6.0.0
> Reporter: Scott Murphy Heiberg
> Priority: Major
>
> {code:groovy}
> class O { def foo() { 'o' } }
> class D { def foo() { 'd' } }
> def c = { x, y -> foo() + x + y }
> assert c.rehydrate(new D(), new O(), new O()).curry(1)(2) == 'o12' // passes
> assert c.curry(1).rehydrate(new D(), new O(), new O())(2) == 'o12' // fails
> {code}
> *Actual:* {{java.lang.ClassCastException: class O cannot be cast to class
> groovy.lang.Closure}}, thrown at {{CurriedClosure.getOwner}} from
> {{CurriedClosure.call}} (on 4.0.33 a MissingMethodException for O.doCall()).
> * {{c.curry(1).dehydrate()}} throws NullPointerException when called.
> * {{c.asWritable().rehydrate(...)}} throws the same ClassCastException.
> * For {{(c >> g).rehydrate(...)}} and {{c.trampoline().rehydrate(...)}} the
> new owner and delegate are ignored: {{foo()}} still resolves against the
> original owner.
> * {{dehydrate()}} on a composed or trampolined closure still holds the
> original owner, so the result cannot be serialized.
> *Expected:* rehydrating or dehydrating a wrapper closure behaves like
> rehydrating or dehydrating the closure it wraps and wrapping the copy.
> Cause: {{Closure.rehydrate}} and {{Closure.dehydrate}} replace only the
> wrapper's own owner, delegate and thisObject fields. {{CurriedClosure}} and
> {{Closure.WritableClosure}} keep the wrapped closure in owner, so it is
> overwritten. {{ComposedClosure}} and {{TrampolineClosure}} call through
> separate fields, so the wrapped closures are never rehydrated or dehydrated.
> Verified on 4.0.33, 5.0.8, 6.0.0 and master.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
