Skip to content

Set transport handlers earlier in Typescript client #12524

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jul 27, 2019
Merged

Set transport handlers earlier in Typescript client #12524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
deed8c7
Set transport handlers earlier in Typescript client
BrennanConroy Jul 24, 2019
af895e3
fix
BrennanConroy Jul 24, 2019
6fc0803
test
BrennanConroy Jul 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions src/SignalR/clients/ts/signalr/src/HttpConnection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ export class HttpConnection implements IConnection {
this.transport = this.constructTransport(HttpTransportType.WebSockets);
// We should just call connect directly in this case.
// No fallback or negotiate in this case.
await this.transport!.connect(url, transferFormat);
await this.startTransport(url, transferFormat);
} else {
throw new Error("Negotiation can only be skipped when using the WebSocket transport directly.");
}
Expand Down Expand Up @@ -281,9 +281,6 @@ export class HttpConnection implements IConnection {
this.features.inherentKeepAlive = true;
}

this.transport!.onreceive = this.onreceive;
this.transport!.onclose = (e) => this.stopConnection(e);

if (this.connectionState === ConnectionState.Connecting) {
// Ensure the connection transitions to the connected state prior to completing this.startInternalPromise.
// start() will handle the case when stop was called and startInternal exits still in the disconnecting state.
Expand Down Expand Up @@ -344,7 +341,7 @@ export class HttpConnection implements IConnection {
if (this.isITransport(requestedTransport)) {
this.logger.log(LogLevel.Debug, "Connection was provided an instance of ITransport, using that directly.");
this.transport = requestedTransport;
await this.transport.connect(connectUrl, requestedTransferFormat);
await this.startTransport(connectUrl, requestedTransferFormat);

return;
}
Expand All @@ -367,7 +364,7 @@ export class HttpConnection implements IConnection {
connectUrl = this.createConnectUrl(url, negotiateResponse.connectionId);
}
try {
await this.transport!.connect(connectUrl, requestedTransferFormat);
await this.startTransport(connectUrl, requestedTransferFormat);
return;
} catch (ex) {
this.logger.log(LogLevel.Error, `Failed to start the transport '${endpoint.transport}': ${ex}`);
Expand Down Expand Up @@ -408,6 +405,12 @@ export class HttpConnection implements IConnection {
}
}

private startTransport(url: string, transferFormat: TransferFormat): Promise<void> {
this.transport!.onreceive = this.onreceive;
this.transport!.onclose = (e) => this.stopConnection(e);
return this.transport!.connect(url, transferFormat);
}

private resolveTransportOrError(endpoint: IAvailableTransport, requestedTransport: HttpTransportType | undefined, requestedTransferFormat: TransferFormat): ITransport | Error {
const transport = HttpTransportType[endpoint.transport];
if (transport === null || transport === undefined) {
Expand Down
78 changes: 78 additions & 0 deletions src/SignalR/clients/ts/signalr/tests/HttpConnection.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -795,6 +795,84 @@ describe("HttpConnection", () => {
});
});

it("transport handlers set before start", async () => {
await VerifyLogger.run(async (logger) => {
const availableTransport = { transport: "LongPolling", transferFormats: ["Text"] };
let handlersSet = false;

let httpClientGetCount = 0;
const options: IHttpConnectionOptions = {
...commonOptions,
httpClient: new TestHttpClient()
.on("POST", () => ({ connectionId: "42", availableTransports: [availableTransport] }))
.on("GET", () => {
httpClientGetCount++;
if (httpClientGetCount === 1) {
if ((connection as any).transport.onreceive && (connection as any).transport.onclose) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a fragile way to test if the handlers are set considering it would fail if the private "transport" property gets renamed. Even the test name of "transport handlers set before start" seems overly specific to the implementation.

Can't we verify this in some more robust way? Like by sending a message as part of the first get response and ensuring the correct .on handler gets called?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a fragile way to test if the handlers are set considering it would fail if the private "transport" property gets renamed.

Says the guy that did (hubConnection as any).serverTimeout(); in tests!

Even the test name of "transport handlers set before start" seems overly specific to the implementation.

Not sure what you mean here, we're literally testing the specific implementation so...

Can't we verify this in some more robust way?

I couldn't find an easy way (read: any way) to test this without grabbing internal fields. In general we don't test race conditions unless there is a way to force the race to happen. In this case I found a way to force it but have to grab the internal fields to verify they were set. If it makes you happy I can just remove the test.

Like by sending a message as part of the first get response and ensuring the correct .on handler gets called?

That doesn't work because the real first response is after connect returns to HttpConnection and by then it's too late as we'll be past the race.

Copy link
Member

@halter73 halter73 Jul 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Says the guy that did (hubConnection as any).serverTimeout(); in tests!

I was actually wondering if you would bring that up when I wrote this. I'm impressed you remembered 😄

Sometimes pragmatism beats purity. I didn't want to force a client test to wait for a server timeout to be exceeded just to pass. I'm a stickler for making tests run quickly, so I thought it was a good tradeoff.

If this really is too difficult to test any other way, I'm fine with it. I'm not entirely convinced that this is a hard to force race condition. You just need a transport that calls one of the callbacks prior to the start promise completion. That would probably be easiest with a mock transport inside a unit test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You just need a transport that calls one of the callbacks prior to the start promise completion. That would probably be easiest with a mock transport inside a unit test.

So, we actually might want to have another test that does this, unfortunately it will hit a different code path than the main one as we check if someone passes in a custom ITransport and go down a different path for that case. But that doesn't help make this test nicer 😢

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine if we just test the custom ITransport in the unit test and leave this test as is.

handlersSet = true;
}
// First long polling request must succeed so start completes
return "";
}
})
.on("DELETE", () => new HttpResponse(202)),
logger,
} as IHttpConnectionOptions;

const connection = new HttpConnection("http://tempuri.org", options);
connection.onreceive = () => null;
try {
await connection.start(TransferFormat.Text);
} finally {
await connection.stop();
}

expect(handlersSet).toBe(true);
});
});

it("transport handlers set before start for custom transports", async () => {
await VerifyLogger.run(async (logger) => {
const availableTransport = { transport: "Custom", transferFormats: ["Text"] };
let handlersSet = false;
const transport: ITransport = {
connect: (url: string, transferFormat: TransferFormat) => {
if (transport.onreceive && transport.onclose) {
handlersSet = true;
}
return Promise.resolve();
},
onclose: null,
onreceive: null,
send: (data: any) => Promise.resolve(),
stop: () => {
if (transport.onclose) {
transport.onclose();
}
return Promise.resolve();
},
};

const options: IHttpConnectionOptions = {
...commonOptions,
httpClient: new TestHttpClient()
.on("POST", () => ({ connectionId: "42", availableTransports: [availableTransport] })),
logger,
transport,
} as IHttpConnectionOptions;

const connection = new HttpConnection("http://tempuri.org", options);
connection.onreceive = () => null;
try {
await connection.start(TransferFormat.Text);
} finally {
await connection.stop();
}

expect(handlersSet).toBe(true);
});
});

describe(".constructor", () => {
it("throws if no Url is provided", async () => {
// Force TypeScript to let us call the constructor incorrectly :)
Expand Down