fix: ensure_async should not silently eat errors and return the coro instead of the result

[maartenbreddels]

We are now returning the coroutine itself, instead of the results of the coroutine.

This gets triggered when a coroutine that is wrapped actually awaits the same coroutine twice. In my case in voila, this caused kernel_id https://github.com/voila-dashboards/voila/blob/4b323460fd5d7e6a7203ff0492607c13c8eb6f25/voila/handler.py#L166 to be assigned the coroutine, because the start_kernel had an exception.

Note that this change causes the tests to fail, I'm not sure how to fix this.

[davidbrochart]

The code you removed was here to handle the error that you see in the tests: ensure_async is passed a coroutine that was already awaited, so you can't await it again.

[maartenbreddels]

Yes, but with the current code, the second call will return a coro, which I assume is always True, so the logic will not be correct. Testing this:

I think it doesn't make sense we cannot call is_alive twice, should we fix that instead?

[davidbrochart]

this caused kernel_id https://github.com/voila-dashboards/voila/blob/4b323460fd5d7e6a7203ff0492607c13c8eb6f25/voila/handler.py#L166 to be assigned the coroutine, because the start_kernel had an exception.

But in this case ensure_async should have raised the exception, and kernel_id should not be assigned the coroutine, right?

[maartenbreddels]

No, because the exception raised was RuntimeException - cannot reuse already awaited coroutine, so ensure_async returned the coro

[davidbrochart]

So this means that self.kernel_manager.start_kernel() returned twice the same coroutine? Are you working on the Voila feature that pre-launches kernels by any chance? If that's the case, I suspect that the second time you call it you are passing the coroutine itself instead of its return value.

[maartenbreddels]

So this means that self.kernel_manager.start_kernel() returned twice the same coroutine?

Almost :), inside start_kernel I accidentally awaited a coro twice, and indeed, this is for https://github.com/voila-dashboards/hotpot_km/pull/2. Note that this was a programmer mistake, not a runtime issue, but it didn't surface (the exception was not raised due to ensure_async). This costed me a lot of time, so I thought we probably want to avoid this in the future.

[davidbrochart]

I agree that the code you removed in this PR should not have been there, and it was mostly to fix an issue that I didn't fully understand. We should probably fix is_alive() as you mentioned, because it shouldn't return twice the same coroutine.

[maartenbreddels]

Yeah, I don't understand why it happens, I've taken a quick look, but cannot find the issue. I hope you have some idea.

(from mobile phone)

On Fri, Mar 5, 2021, 17:24 David Brochart [email protected] wrote:

I agree that the code you removed in this PR should not have been there, and it was mostly to fix an issue that I didn't fully understand. We should probably fix is_alive() as you mentioned, because it shouldn't return twice the same coroutine.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jupyter/nbclient/pull/138#issuecomment-791527226, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANPEPNMZ7JUPVJCWYMX4TTTCEASZANCNFSM4YVE5JUA .

[davidbrochart]

I've looked too, but jupyter-client is becoming really complicated. I opened https://github.com/jupyter/jupyter_client/issues/621.