Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement auto reconnect on SignalR TypeScript client #8566

Merged
merged 14 commits into from Apr 8, 2019

Conversation

Projects
None yet
7 participants
@halter73
Copy link
Member

commented Mar 15, 2019

Partially addresses #5282

Fixes #8709 (added by @anurse :trollface:)

@anurse anurse added this to the 3.0.0-preview4 milestone Mar 21, 2019

@halter73 halter73 force-pushed the halter73/5282 branch 2 times, most recently from 1a548a4 to aa6b0e5 Mar 25, 2019

@halter73 halter73 force-pushed the halter73/5282 branch 5 times, most recently from afbd63e to de62cf7 Apr 2, 2019

@halter73 halter73 marked this pull request as ready for review Apr 3, 2019

@halter73

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

@JamesNK @mikaelm12 @BrennanConroy @anurse This is ready for review.

I only plan to add two more tests similar to AutomaticReconnect.test.ts's "does not reset after hub handshake failure" test that tests the integration between HubConnection and HttpConnection.

  1. A test to verify the onreconnected connectionId parameter appropriately provides the new connection id.
  2. A test to verify that the client can reconnect again after it successfully and fully reconnects once.

I'm also considering adding a test to verify the reconnect delay timeout gets cleared when the connection stops, but making the delay long enough to be reliably but quick enough not to slow down our tests might be tricky.

@halter73 halter73 force-pushed the halter73/5282 branch 3 times, most recently from 2ce6a6b to 728a368 Apr 4, 2019

@halter73

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

@halter73 halter73 changed the title Start implementing auto reconnect on SignalR TypeScript client Implement auto reconnect on SignalR TypeScript client Apr 5, 2019

@BrennanConroy
Copy link
Member

left a comment

I'll review more tonight/tomorrow

Show resolved Hide resolved src/SignalR/clients/ts/signalr/src/HttpConnection.ts
private stopConnection(error?: Error): void {
this.logger.log(LogLevel.Debug, `HttpConnection.stopConnection(${error}) called while in sate ${this.connectionState}.`);

This comment has been minimized.

Copy link
@BrennanConroy

BrennanConroy Apr 6, 2019

Member
Suggested change
this.logger.log(LogLevel.Debug, `HttpConnection.stopConnection(${error}) called while in sate ${this.connectionState}.`);
this.logger.log(LogLevel.Debug, `HttpConnection.stopConnection(${error}) called while in state ${this.connectionState}.`);

This comment has been minimized.

Copy link
@BrennanConroy

This comment has been minimized.

Copy link
@BrennanConroy

BrennanConroy Apr 7, 2019

Member

Third times a charm
"sate" -> "state"

Show resolved Hide resolved src/SignalR/clients/ts/signalr/src/HttpConnection.ts
Show resolved Hide resolved src/SignalR/clients/ts/signalr/src/HubConnectionBuilder.ts
Show resolved Hide resolved src/SignalR/clients/ts/signalr/tests/HttpConnection.test.ts
Show resolved Hide resolved src/SignalR/clients/ts/signalr/tests/Utils.ts Outdated
Show resolved Hide resolved src/SignalR/clients/ts/signalr/src/HubConnection.ts

@halter73 halter73 force-pushed the halter73/5282 branch from 980b858 to 9f4db9d Apr 6, 2019

@mikaelm12
Copy link
Contributor

left a comment

I think this looks okay for preview 4. We can refine this in the next preview as well. Let's just wrap up the testing

private stopConnection(error?: Error): void {
this.logger.log(LogLevel.Debug, `HttpConnection.stopConnection(${error}) called while in sate ${this.connectionState}.`);

This comment has been minimized.

Copy link
@BrennanConroy
Show resolved Hide resolved src/SignalR/clients/ts/signalr/src/HubConnection.ts Outdated
Show resolved Hide resolved src/SignalR/clients/ts/signalr/src/HubConnection.ts Outdated
Show resolved Hide resolved src/SignalR/clients/ts/signalr/src/HubConnection.ts Outdated
Show resolved Hide resolved src/SignalR/clients/ts/signalr/src/HubConnection.ts Outdated
Show resolved Hide resolved src/SignalR/clients/ts/signalr/src/HubConnection.ts Outdated
Show resolved Hide resolved src/SignalR/clients/ts/signalr/src/HubConnection.ts
@BrennanConroy
Copy link
Member

left a comment

Looks like there might be a flaky/always failing test?

@halter73 halter73 force-pushed the halter73/5282 branch from 2818295 to 6765874 Apr 7, 2019

@BrennanConroy
Copy link
Member

left a comment

Should the onclosed callback be triggered if the connection stops while in the reconnecting state?
We should file an issue for adding some random delay.
We should file an issue for figuring out how to make reconnect work nicely with handshake failure.
Some of these logs might be better suited as trace.

Connecting,
Connected,
Disconnected,
Connecting = "Connecting ",

This comment has been minimized.

Copy link
@BrennanConroy

BrennanConroy Apr 7, 2019

Member

Nice 👍

@halter73 halter73 force-pushed the halter73/5282 branch from 2112031 to 7dba57b Apr 7, 2019

@halter73 halter73 force-pushed the halter73/5282 branch 3 times, most recently from 2ed3eb5 to a791b63 Apr 8, 2019

@halter73 halter73 force-pushed the halter73/5282 branch from a791b63 to f10f8b8 Apr 8, 2019

@halter73 halter73 merged commit 74bba27 into master Apr 8, 2019

15 checks passed

AspNetCore-ci Build #20190408.8 succeeded
Details
AspNetCore-ci (Build: Linux ARM) Build: Linux ARM succeeded
Details
AspNetCore-ci (Build: Linux ARM64) Build: Linux ARM64 succeeded
Details
AspNetCore-ci (Build: Linux Musl x64) Build: Linux Musl x64 succeeded
Details
AspNetCore-ci (Build: Linux x64) Build: Linux x64 succeeded
Details
AspNetCore-ci (Build: Windows ARM) Build: Windows ARM succeeded
Details
AspNetCore-ci (Build: Windows x64/x86) Build: Windows x64/x86 succeeded
Details
AspNetCore-ci (Build: macOS) Build: macOS succeeded
Details
AspNetCore-ci (Code check) Code check succeeded
Details
AspNetCore-ci (Test: Templates - Windows Server 2016 x64) Test: Templates - Windows Server 2016 x64 succeeded
Details
AspNetCore-ci (Test: Ubuntu 16.04 x64) Test: Ubuntu 16.04 x64 succeeded
Details
AspNetCore-ci (Test: Windows Server 2016 x64) Test: Windows Server 2016 x64 succeeded
Details
AspNetCore-ci (Test: macOS 10.13) Test: macOS 10.13 succeeded
Details
AspNetCore-helix-test Build #20190408.8 succeeded
Details
license/cla All CLA requirements met.
Details

@halter73 halter73 deleted the halter73/5282 branch Apr 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.