Skip to content

Reproduce problem in issue 1439 - #1440

Closed
skjolber wants to merge 1 commit into
mrousavy:mainfrom
skjolber:issue1439
Closed

Reproduce problem in issue 1439#1440
skjolber wants to merge 1 commit into
mrousavy:mainfrom
skjolber:issue1439

Conversation

@skjolber

Copy link
Copy Markdown

This was an interesting AI ride - in the end:

The bug is now perfectly reproduced. The logcat confirms:

1. First call throws as expected:  "First call threw as expected: ...Expected error on first call"   
2. Second invocation starts but never returns:  "Starting second invocation..." 
3. After 3 seconds timeout:  "Issue #1439: second fn invocation timed out — SafeContinuation swallowed the resume, CompletableFuture.get() blocks forever" 

Root cause confirmed: When  inner.await(new CustomContinuation<>(future2))  is called and  inner  is already resolved, Kotlin's  SafeContinuation  returns the value directly from  await()  without ever calling  CustomContinuation.resumeWith() . The  CompletableFuture  is never completed, causing  future2.get()  to block forever.

Summary of what was created:

•  CustomContinuation.java  — exact replica of the abt SDK pattern that triggers the bug
•  JavaCallHelper.java  — Java implementation of  fn.invoke()  →  call.await(continuation)  →  future.get()  chain  
•  HybridIssue1439.kt  — Kotlin implementation using  JavaCallHelper  to reproduce the hang
• Tests fail with  "Timeouted!"  (✕), matching the real-world bug behavior

Me: is this a bug in the nitro or in the java implementation?

Thought for 22s
The user is asking whether the bug is in Nitro itself or in the Java implementation (the abt SDK / the CustomContinuation pattern).
Let me think about this clearly based on my analysis.
The root cause is:
- Kotlin's `suspend fun await()` uses `SafeContinuation` internally
- When called from Java with a custom `Continuation`, if the Promise is already resolved, `SafeContinuation` returns the value directly from `await()` without calling the custom continuation's `resumeWith`
- The Java code ignores the return value and calls `future.get()`, which blocks forever
- The `Promise.await()` Kotlin suspend function is part of Nitro
- `SafeContinuation` behaves correctly for normal coroutine usage, but calling a suspend function from Java with a custom continuation is fundamentally incompatible with how Kotlin handles synchronous resolution
- Nitro could provide a Java-friendly API on `Promise<T>` that doesn't rely on `suspendCoroutine` to avoid this pitfall
- The abt SDK is using an anti-pattern by calling a Kotlin suspend function directly from Java with a custom `Continuation`, which breaks when the promise resolves synchronously
- The proper solution would be to use a callback-based approach like `call.then()` instead
, which Nitro already supports
The real issue is both a usage anti-pattern and a gap in Nitro's API design. Calling `await()` from Java with a custom `Continuation` isn't how Kotlin coroutines are meant to be used from non-coroutine code. Nitro should either document this limitation or provide a dedicated blocking API for Java callers. The memory leak in `JPromise` where listeners aren't cleared is a separate concern but not the root cause of the hang.

This is primarily a Java usage anti-pattern, but Nitro shares some responsibility.

The Java anti-pattern (root cause)

 Promise.await()  is a Kotlin  suspend fun . Calling it from Java with a custom  Continuation  is not a supported use case:

// ANTI-PATTERN: calling a suspend fun from Java with a raw Continuation
inner.await(new CustomContinuation<>(future2));  // ← wrong
future2.get();                                   // ← hangs when inner already resolved

Kotlin's  SafeContinuation  (used internally by  suspendCoroutine ) returns the value directly from  await()  when the Promise is already resolved — without ever calling  CustomContinuation.resumeWith() . Java ignores the return value, so  future2  is never completed.

The correct Java/non-coroutine approach — which Nitro already provides — is:

inner.then(v  -> future2.complete(v));          // ✅ always fires
inner.getCatch(e -> future2.completeExceptionally(e));
future2.get();  // works correctly

Nitro's partial responsibility

 JPromise  has a related bug:  addOnResolvedListenerJava()  on an already-rejected Promise silently queues the listener in  _onResolvedListeners , which is never called (memory leak). The symmetric bug exists for rejected listeners on a resolved Promise. This should be fixed in  JPromise.cpp :

void addOnResolvedListener(OnResolvedFunc&& onResolved) {
   std::unique_lock lock(_mutex);
   if (auto result = std::get_if<ResultType>(&_state)) {
       onResolved(*result);          // already resolved → call immediately ✓
   } else if (std::holds_alternative<ErrorType>(_state)) {
       /* already rejected → listener would never fire, drop it */
   } else {
       _onResolvedListeners.push_back(std::move(onResolved));
   }
}

Also,  JPromise::resolve() / reject()  don't clear the opposite listener list after firing — unlike  Promise<T>  which does  std::move(_onRejectedListeners)  (clearing it). That's another minor inconsistency.

Bottom line

┌────────────────────────────────────────────────────────┬─────────────────────────────┐
│ Issue                                                  │ Where                       │
├────────────────────────────────────────────────────────┼─────────────────────────────┤
│ Calling suspend fun from Java with raw Continuation    │ Java/abt SDK (anti-pattern) │
├────────────────────────────────────────────────────────┼─────────────────────────────┤
│ JPromise listener leak on wrong-state registration     │ Nitro (minor bug)           │
├────────────────────────────────────────────────────────┼─────────────────────────────┤
│ JPromise doesn't clear opposite listeners after settle │ Nitro (inconsistency)       │
└────────────────────────────────────────────────────────┴─────────────────────────────┘

The fix for the abt SDK: replace  call.await(new CustomContinuation<>(future))  with  call.then(v -> future.complete(v))  +  call.getCatch(e -> future.completeExceptionally(e)) .


So I guess the main result is that my await call is the culpit. The AI claims there is a minor bug in JPromise if that is something you would like to look into. Otherwise just close this PR / issue.

@vercel

vercel Bot commented Jul 17, 2026

Copy link
Copy Markdown

@skjolber is attempting to deploy a commit to the Margelo Team on Vercel.

A member of the Team first needs to authorize it.

@mrousavy

Copy link
Copy Markdown
Owner

First of all, thanks for providing a reproduction.

I know this is just for testing, but I have never seen such ugly test plumbing code before - this is all AI generated right?
The tests have proper documentation, a README.md, and guidelines even written for AI to teach it how to write tests for Nitro. Yet it completely ignored all of that and chose to build a whole new test file instead of adding stuff to the existing test file, created whole new Hybrid Objects from scratch, new structs, etc all completely new and not reusing any of the existing stuff.
Also your committed .harness files like manifest or a crash trace.

May I please ask you to write a proper test for this using the existing tests we have? This should NOT be a 41 file/+2,257 lines PR.

@mrousavy

Copy link
Copy Markdown
Owner

I will fix such async calls - but first we need a proper test added here in as little lines as needed then I can start working from there.

@skjolber

Copy link
Copy Markdown
Author

@mrousavy Yes this is all AI. Not sure why the AI (clause sonnet) did not pick up the instructions.

Closed this PR in favor of #1441, which it also all AI.

@skjolber skjolber closed this Jul 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants