Skip to content

Step 1 (PR-B): Async project defensive coding cleanup#15682

Open
jamesfredley wants to merge 2 commits into
8.0.xfrom
chore/async-defensive-cleanup
Open

Step 1 (PR-B): Async project defensive coding cleanup#15682
jamesfredley wants to merge 2 commits into
8.0.xfrom
chore/async-defensive-cleanup

Conversation

@jamesfredley
Copy link
Copy Markdown
Contributor

Step 1 (PR-B) prerequisite for Hibernate 7 work - extracted from the staging branch so it can be reviewed on its own merits.

Context

This change was originally pulled forward into the hibernate7 staging branch as commit e3cad414db on PR #15654. Reviewers (@matrei, @sbglasius) correctly pointed out it is unrelated to the Hibernate 7 clone and should be a standalone PR. Extracting it here against 8.0.x so it can land on its own and then flow naturally into both the staging branch (PR #15654) and Step 2 (PR #15568).

Scope

Nine files in grails-async/ with defensive coding improvements:

1. Null-safety guards

2. Exception handling

  • DelegateAsyncTransformation.getTransformer(): catch Exception (renamed to ignored) instead of Throwable. Catching Throwable swallows Errors that should propagate (OutOfMemoryError, StackOverflowError, etc.) and is widely considered bad style. @sbglasius called this out explicitly in his review of the staging PR.

3. Static analysis hints

  • Add @Override annotations on DelegateAsyncTransformation.visit and NoopDelegateAsyncTransactionalMethodTransformer.transformTransactionalMethod.
  • Use constant-on-left equality (VOID.equals(name) instead of name.equals(VOID)) to avoid NPE if name is null.
  • Change copyParameters(Parameter[]) to varargs copyParameters(Parameter...) for caller convenience.

Why a separate PR

PR #15654 (Step 1) is meant to be a near-pure clone of hibernate5hibernate7. PR #15568 (Step 2) is the actual Hibernate 7 logic. Both reviewers asked for cleanup of this nature to be split out so the review effort on Step 1 and Step 2 can stay focused on hibernate-related diffs.

Authorship preserved from the original commit by @jdaugherty.

Related

  • Step 1 PR-A: Hibernate 7 - Step 1 #15654 (the hibernate7 clone, already updated to remove logback.groovy and the unrelated stepByStep.adoc change)
  • Step 1 PR-C: MongoDatastoreSpec base class + spec refactor (forthcoming)
  • Step 1 PR-D: DetachedCriteriaSpec TCK style cleanup (forthcoming)
  • Step 2: Hibernate 7 - Step 2 #15568 (the actual Hibernate 7 logic - blocked on Step 1 prerequisites)

Defensive coding improvements to the grails-async module:

1. Null-safety guards
   - AbstractPromiseFactory.createPromise(List, List): return an empty
     PromiseList when the closures list is null rather than NPE on size().
   - FutureTaskPromise.set(T) and setException(Throwable): null-check
     successCallbacks and failureCallbacks before synchronizing/iterating.

2. Exception handling
   - DelegateAsyncTransformation.getTransformer(): catch Exception
     (renamed to ignored) instead of Throwable. Catching Throwable
     swallows Errors that should propagate (OutOfMemoryError,
     StackOverflowError, etc.) and is widely considered bad style.

3. Static analysis hints
   - Add @OverRide on DelegateAsyncTransformation.visit and on the
     NoopDelegateAsyncTransactionalMethodTransformer.transformTransactionalMethod
     overrides.
   - Use constant-on-left equality (VOID.equals(name) instead of
     name.equals(VOID)) to avoid NPE if name is null.
   - Change copyParameters(Parameter[]) to varargs
     copyParameters(Parameter...) for caller convenience.

This change was originally pulled forward into the hibernate7 staging
branch (PR #15654 commit e3cad41) and was flagged by reviewers as
unrelated to the Hibernate 7 clone. Extracting it here so it can land
on 8.0.x on its own merits as a prerequisite for the Hibernate 7 work.

Assisted-by: claude-code:claude-4.7-opus
Copilot AI review requested due to automatic review settings May 27, 2026 21:03
Copy link
Copy Markdown
Contributor

@jdaugherty jdaugherty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes were originally authored by walter. I believe they were all issues found by PMD

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Defensive coding and static-analysis cleanups across grails-async/ as a prerequisite for upcoming Hibernate 7 work, focusing on null-safety, safer exception handling, and minor correctness/clarity improvements.

Changes:

  • Add null-safety handling for promise list creation and callback invocation.
  • Refine DelegateAsyncTransformation (exception handling scope, @Override, safer equality, varargs signature).
  • Minor refactors/cleanups (import ordering, constant immutability, formatting).

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/spring/PromiseFactoryBean.groovy Removes stray trailing whitespace/blank line.
grails-async/plugin/src/main/groovy/org/grails/plugins/web/async/AsyncWebRequestPromiseDecorator.groovy Import order cleanup.
grails-async/plugin/src/main/groovy/org/grails/async/transform/internal/DefaultDelegateAsyncTransactionalMethodTransformer.groovy Minor formatting of parameter array constant; import order.
grails-async/plugin/src/main/groovy/grails/async/web/AsyncGrailsWebRequest.groovy Import ordering cleanup (no functional change).
grails-async/plugin/src/main/groovy/grails/async/services/PersistenceContextPromiseDecorator.groovy Import order cleanup.
grails-async/gpars/src/main/groovy/org/grails/async/factory/gpars/LoggingPoolFactory.groovy Makes reflective Method field final (immutable after static init).
grails-async/core/src/main/groovy/org/grails/async/transform/internal/DelegateAsyncTransformation.java Adds @Override, safer string equality, narrows catch to Exception, changes helper to varargs.
grails-async/core/src/main/groovy/org/grails/async/factory/future/FutureTaskPromise.groovy Adds null-guards around callback iteration.
grails-async/core/src/main/groovy/grails/async/factory/AbstractPromiseFactory.groovy Adds null-handling for createPromise(List, List) by returning an “empty” promise result.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Fixes four Copilot review comments on the async defensive cleanup:

1. AbstractPromiseFactory.createPromise(List, List) on null input
   Previously returned `new PromiseList<T>()`. An empty PromiseList delegates
   `onComplete`/`onError` to the active PromiseFactory's
   `onComplete(List, Closure)`, which (in CachedThreadPoolPromiseFactory)
   uses `while (promises.every { !promise.isDone() })`. Because Groovy's
   `every` returns `true` for an empty collection (vacuous truth), this
   loop would never terminate and any caller registering a completion
   handler on the empty promise would busy-wait forever.

   Now returns `new BoundPromise<List<T>>(Collections.<T> emptyList())`,
   which is an already-resolved promise that invokes onComplete callbacks
   synchronously with the empty list value.

2. FutureTaskPromise.set / setException null-checks removed
   `successCallbacks` and `failureCallbacks` are declared `final` and
   initialized at the field declaration with `new ConcurrentLinkedQueue<>()`.
   They cannot be null, so the defensive null-checks added in the previous
   commit were dead code and misleading about the field nullability.

3. Add regression test for the null-closures path
   New spec method in SynchronousPromiseFactorySpec verifies that
   `createPromise((List)null, decorators)` returns an already-done promise
   resolving to an empty list and that an `onComplete` callback registered
   on it is invoked synchronously with the empty list rather than
   busy-waiting.

Assisted-by: claude-code:claude-4.7-opus
jamesfredley added a commit that referenced this pull request May 28, 2026
This commit reverts the portions of this branch that have been split out
into standalone PRs against 8.0.x. The goal is to shrink the PR-A
review surface to focus on the actual hibernate5 -> hibernate7 clone
and let the unrelated cleanups be reviewed on their own merits.

Once PRs B/C/D/E land on 8.0.x and 8.0.x is merged into this branch,
the reverted changes will arrive through the merge so the final state
of stage-hibernate7 is unchanged - only the diff visible on this PR
is reduced.

Reverted content:

  Async defensive cleanup (PR #15682, PR-B)
    9 files in grails-async/. Reverts e3cad41. Null-safety guards,
    @OverRide annotations, catch(Exception ignored) over catch(Throwable),
    constant-on-left equality, varargs.

  addAllDomainClasses helper + adoption (PR #15683, PR-C)
    Helper method on GrailsDataTckManager plus all callers across the
    data mapping test suites. Reverts ed0916d plus all orphan
    callers added by subsequent commits (50 specs) so the branch
    compiles without the helper method.

  MongoDatastoreSpec base class + mongo package rename (PR #15685, PR-E)
    Reverts c8cb43f (MongoDatastoreSpec introduction + ~107 spec
    refactors) and the mongo portion of 01c27f9 (8 file renames
    grails.gorm.tests -> grails.gorm.specs plus 18 import-update
    modifications in non-renamed mongo specs).

  DetachedCriteriaSpec command-chain syntax (PR #15684, PR-D)
    18 `eq(...)` and `like(...)` parens restorations in the TCK
    DetachedCriteriaSpec. Reverts the parens hunk of ee4ab14 while
    leaving the rest of that commit (setupSpec addition, import
    reorder, and the other 35 TCK file changes) intact.

NOT reverted (intentionally kept in PR-A):

  - The hibernate5 -> hibernate7 baseline clone (~100k lines, the
    actual work being reviewed)
  - 89 hibernate5 and 90 hibernate7 grails.gorm.tests -> grails.gorm.specs
    package renames (non-mongo portion of 01c27f9, since these
    touch the cloned tests and reverting would require re-doing both
    sides)
  - 18 grails-datamapping-core-test package renames (non-mongo portion
    of 01c27f9)
  - All other "Pull forward..." commits (styling, build config,
    test additions, etc.) which the reviewers have not specifically
    objected to

Assisted-by: claude-code:claude-4.7-opus
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants