adamsaghy commented on PR #6363:
URL: https://github.com/apache/fineract/pull/6363#issuecomment-5539916888
## Verdict
Correct fix, and the approach is better than a local guard: it replaces the
`ThreadLocal<Boolean>` with `ThreadLocal<Integer> eventRecordingDepth`, makes
`start`/`stop`/`reset` **private**, and exposes only
`withExternalEventRecording(Supplier|Runnable)`. Since
`BusinessEventNotifierServiceImpl` is the only implementor and all four call
sites (COB, loan replay, WC publisher, tests) are migrated, callers can no
longer reintroduce the non-reentrant shape. CI is fully green.
## One correction to the framing: this is live, not latent
The "latent today (only command paths call reprocessing)" reading holds for
the WC publisher, but the identical
`ReplayedTransactionBusinessEventServiceImpl` shape **is** reachable from
inside COB on `develop` today:
```
LoanInterestRecalculationCOBBusinessStep:51
-> LoanWritePlatformServiceJpaRepositoryImpl.recalculateInterest(Loan)
-> LoanScheduleService:89
reprocessLoanTransactionsService.reprocessTransactions(loan) (non-progressive
branch)
-> ReprocessLoanTransactionsServiceImpl.handleChangedDetail
-> ReprocessLoanTransactionsServiceImpl:155 raiseTransactionReplayedEvents
```
So with `enable-cob-bulk-event` on, a cumulative non-progressive
interest-recalculating loan already flushes COB's window mid-chain, and every
event from the steps after `LOAN_INTEREST_RECALCULATION` escapes as an
individual external event. That is what the new
`FeignCobBulkEventRecordingWindowTest` reproduces end-to-end (with a good
negative control).
Worth stating in the JIRA/PR description — it raises this from hardening to
a bug fix, and it means consumers see a behavior change: replay events during
COB now arrive inside COB's bulk event instead of their own.
## Findings
### 1. `resetEventRecording()` zeroes the counter instead of decrementing it
**Severity: low — hardening, not reachable today.**
The failure path calls `eventRecordingDepth.remove()`, justified by the
comment "the exception propagates past every enclosing window too, so there is
nothing left for them to post either." Nothing enforces that — it is a claim
about callers, which is precisely the kind of assumption that produced the
original bug.
If any frame between an inner window and an enclosing one ever catches and
continues, the enclosing window's depth is gone: its remaining events post
individually, and the *next* nested window becomes "outermost" and flushes a
premature bulk mid-chain.
Verified unreachable right now: `executeBusinessSteps` rethrows as
`BusinessStepException`, the interest-recalc step's `try` has only a `finally`,
and every `@Retry(fallbackMethod=…)` sits in a command handler above any
window. But making the counter symmetric costs nothing:
```java
private void resetEventRecording() {
int depth = eventRecordingDepth.get();
if (depth <= 1) {
eventRecordingDepth.remove();
} else {
eventRecordingDepth.set(depth - 1);
}
recordedEvents.remove();
}
```
The `<= 1` branch keeps the ThreadLocal entry from leaking on pooled
threads, which a plain decrement would not. Behavior on today's paths is
unchanged, and `testARecordingWindowShouldBeUsableAgainAfterAPreviousOneFailed`
still passes.
### 2. Stale comment in the new integration test
In `runCobAndFetchLoanCategoryEvents`, the comment says the Loan-category
filter is needed because "this test-only endpoint cannot render a bulk event
holding more than one item" — but this PR is what makes it able to, and
`bulkEventItemTypes` two methods below reads the multi-item `datas` list. The
filter is still needed (to exclude the `Bulk`-category event from the "escaped"
list); only the stated reason is wrong.
### 3. Cosmetic: test names now overstate what they check
`COBBulkEventConfigurationTest.testGivenBulkEventEnabledWhenCOBRunExceptionThenEventRecordingReset`
and the Cucumber step definition now only assert the wrapper was invoked, not
that recording was reset. That is the right call — the behavior is asserted
directly in
`BusinessEventNotifierServiceImplTest.testFailingRecordingWindowShouldAbandonTheRecordingAndRethrow`,
and the PR adds a comment saying so — but the method names still promise
"…ThenEventRecordingReset".
## Worth mentioning in the description: the bundled
`InternalExternalEventService` fix
The bulk-rendering change is unrelated to the title but is a genuine fix,
not just refactoring. The old code appended each item's JSON separated by
`System.lineSeparator()` and passed the whole blob to `mapper.readValue`; with
`FAIL_ON_TRAILING_TOKENS` off by default, that silently returned only the
**first** item, so the internal endpoint never rendered a multi-item bulk event
correctly. The new `Map.of("datas", bulkItems)` shape fixes it and is what
makes the integration test assertable.
It is an API-shape change, but only to `/v1/internal/externalevents`, which
is `@Profile(TEST)`-gated; no integration test reads bulk payload contents
today, so nothing breaks. It just deserves a line in the description rather
than arriving unannounced.
## Also worth noting
The `Runnable`/`Supplier` overload pair resolves as intended (JLS 15.12.2.5
makes the non-void function type more specific, so COB's value-returning lambda
binds to `Supplier`), but it means a `Runnable`-intended lambda whose body
happens to be a value-returning expression — `() -> list.add(x)` — silently
binds to `Supplier`. Harmless here, since both overloads do the same thing.
--
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]
