Support async callbacks in SignalR typescript client

sryarama commented 1 week ago

Is there an existing issue for this?

[X] I have searched the existing issues

Describe the bug

We have async listeners set up for onclose, onreconnecting, and onreconnected events, along with a logger to capture all SignalR messages. However, when an unhandled rejection occurs within the async listeners, it is not being passed back to the registered logger.

Expected Behavior

No response

Steps To Reproduce

No response

Exceptions (if any)

No response

.NET Version

No response

Anything else?

No response

BrennanConroy commented 1 week ago

Please show an example. We don't even know what client you're using.

sryarama commented 1 week ago

Here is the sample listener code.

hubConnection.onclose( async (error) => { if (isTokenValid) { throw new Error("Invalid token"); } else { await reconnect(); } });

Lets say if I don't have a valid token, I want to log an error in the telemetry. SignalR SDK is capturing the exception I throw above and not sending it through the logger I registered during initialization.

BrennanConroy commented 1 week ago

Ok so you're using the typescript library. The exception is being passed to the logger: https://github.com/dotnet/aspnetcore/blob/80ac2ff6a4e223eb5a91112912dc5d53c093d47f/src/SignalR/clients/ts/signalr/src/HubConnection.ts#L864 I'm guessing you didn't call configureLogging(logger) on the HubConnectionBuilder. But also, if you're explicitly throwing an exception just for it to be logged, why not just log instead of throwing.

sryarama commented 1 week ago

We registered the logger using configureLogging. And it is working as expected when I register the sync listeners like this hubConnection.onclose((error) => { });. Issue is only with async listeners hubConnection.onclose(async (error) => { });

About explicitly throwing, its not that simple in our case. For easy understanding I gave the example like that.

BrennanConroy commented 1 week ago

Oh I see, missed the fact that sync works and async doesn't. Async methods aren't currently supported for the callbacks. See the method signature: public onclose(callback: (error?: Error) => void): void If you used a linter in typescript you might see warnings/errors when passing an async method to onclose. https://typescript-eslint.io/rules/no-misused-promises/

Obviously I don't know your code and how much control you have, but wrapping anything passed to onclose(...) in a try {} catch{} would fix your issue.

AshkanAfsharpour commented 1 week ago

Hi @BrennanConroy,

After reviewing the current implementation of the SignalR TypeScript client, I’ve identified that the method signatures for onclose, onreconnecting, and onreconnected currently do not support async callbacks.

To resolve this, I propose updating the method signatures to support Promise for these callbacks, as shown below:

public onclose(callback: (error?: Error) => void | Promise<void>): void;

Additionally, the _completeClose method should be made async and updated to properly await both synchronous and asynchronous callbacks. The current implementation: https://github.com/dotnet/aspnetcore/blob/80ac2ff6a4e223eb5a91112912dc5d53c093d47f/src/SignalR/clients/ts/signalr/src/HubConnection.ts#L861-L865

Would be updated to this:

try {
    for (const callback of this._closedCallbacks) {
        await callback(error);
    }
} catch (e) {
    this._logger.log(LogLevel.Error, `An onclose callback called with error '${error}' threw error '${e}'.`);
}

I would be happy to submit a PR for this change, along with tests to cover both synchronous and asynchronous scenarios.

dotnet-policy-service[bot] commented 1 week ago

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

halter73 commented 1 week ago

API Review Notes:

Seems convenient. As noted, this mostly already works, but we should definitely update the logging logic to await if we're updating the signature.

API Approved!

- public onclose(callback: (error?: Error) => void): void;
+ public onclose(callback: (error?: Error) => void | Promise<void>): void;

- public onreconnecting(callback: (error?: Error) => void): void;
+ public onreconnecting(callback: (error?: Error) => void | Promise<void>): void;

- public onreconnected(callback: (connectionId?: string) => void): void;
+ public onreconnected(callback: (connectionId?: string) => void | Promise<void>): void;

BrennanConroy commented 1 week ago

try { for (const callback of this._closedCallbacks) { await callback(error); } } catch (e) { this._logger.log(LogLevel.Error, An onclose callback called with error '${error}' threw error '${e}'.); }

We need to be careful about just awaiting inline. It would mean marking the entire method as async which then means awaiting it at all the call-sites. We might just want to fire-and-forget these callbacks. For example see the C# client https://github.com/dotnet/aspnetcore/blob/54c0cc8fa74e8196a2ce0711a20959143be7fb6f/src/SignalR/clients/csharp/Client.Core/src/HubConnection.cs#L1795-L1796

AshkanAfsharpour commented 1 week ago

Hey @halter73 and @BrennanConroy,

Thanks for the feedback! i get the concern about awaiting the callbacks directly and making the whole _completeClose method async. Instead of that, im thinking of taking the fire-and-forget approach similar to what the C# client does. Here is what im proposing:

Update the method signatures as we discussed:

public onclose(callback: (error?: Error) => void | Promise<void>): void;
public onreconnecting(callback: (error?: Error) => void | Promise<void>): void;
public onreconnected(callback: (connectionId?: string) => void | Promise<void>): void;

Instead of making _completeClose async, I'll keep it sync and use a seperate _runCloseCallbacks function to handle both sync and async callbacks. This way, we dont need to worry about changing all the call sites:

private _completeClose(error?: Error): void {
    if (this._connectionStarted) {
        this._connectionState = HubConnectionState.Disconnected;
        this._connectionStarted = false;
        if (this._messageBuffer) {
            this._messageBuffer._dispose(error ?? new Error("Connection closed."));
            this._messageBuffer = undefined;
        }

        if (Platform.isBrowser) {
            window.document.removeEventListener("freeze", this._freezeEventListener);
        }

        // Fire-and-forget the callbacks
        this._runCloseCallbacks(error);
    }
}

private async _runCloseCallbacks(error?: Error): Promise<void> {
    try {
        for (const callback of this._closedCallbacks) {
            await callback(error); // Await both sync and async callbacks
        }
    } catch (e) {
        this._logger.log(LogLevel.Error, `An onclose callback called with error '${error}' threw error '${e}'.`);
    }
}

And also do the same for reconnecting and reconnected callbacks. https://github.com/dotnet/aspnetcore/blob/80ac2ff6a4e223eb5a91112912dc5d53c093d47f/src/SignalR/clients/ts/signalr/src/HubConnection.ts#L891-L896

https://github.com/dotnet/aspnetcore/blob/80ac2ff6a4e223eb5a91112912dc5d53c093d47f/src/SignalR/clients/ts/signalr/src/HubConnection.ts#L924-L928

Let me know if this looks good, and I’ll go ahead and submit the PR.

dotnet / aspnetcore