From 4e8598d6e0c5e1c906308a65533f0324706532c7 Mon Sep 17 00:00:00 2001 From: Marcus Longmuir Date: Fri, 11 Oct 2019 10:56:43 +0100 Subject: [PATCH 1/7] Removed detach --- .../grpc-web-fake-transport/src/index.spec.ts | 41 ++---- client/grpc-web/src/client.ts | 23 +-- client/grpc-web/src/detach.ts | 42 +----- client/grpc-web/src/transports/http/fetch.ts | 21 +-- client/grpc-web/src/transports/http/xhr.ts | 21 +-- .../src/transports/websocket/websocket.ts | 13 +- integration_test/ts/src/detach.spec.ts | 131 ------------------ integration_test/ts/src/spec.ts | 3 +- 8 files changed, 37 insertions(+), 258 deletions(-) delete mode 100644 integration_test/ts/src/detach.spec.ts diff --git a/client/grpc-web-fake-transport/src/index.spec.ts b/client/grpc-web-fake-transport/src/index.spec.ts index a4b5e1ca..8a80ef56 100644 --- a/client/grpc-web-fake-transport/src/index.spec.ts +++ b/client/grpc-web-fake-transport/src/index.spec.ts @@ -227,7 +227,7 @@ describe("FakeTransportBuilder", () => { }); describe("manual trigger", () => { - it("should allow the consumer to control the lifecycle of the server response", done => { + it("should allow the consumer to control the lifecycle of the server response", () => { const onHeadersSpy = jest.fn(); const onMessageSpy = jest.fn(); const onEndSpy = jest.fn(); @@ -258,30 +258,19 @@ describe("FakeTransportBuilder", () => { transport.sendHeaders(); - // Note that we must defer the assertions as the grpc-web package detaches all callbacks, so they - // will not run immediately. - setTimeout(() => { - expect(onHeadersSpy).toHaveBeenCalled(); - expect(onMessageSpy).not.toHaveBeenCalled(); - expect(onEndSpy).not.toHaveBeenCalled(); - - transport.sendMessages(); - setTimeout(() => { - expect(onHeadersSpy).toHaveBeenCalled(); - expect(onMessageSpy).toHaveBeenCalled(); - expect(onEndSpy).not.toHaveBeenCalled(); - - transport.sendTrailers(); - setTimeout(() => { - expect(onHeadersSpy).toHaveBeenCalled(); - expect(onMessageSpy).toHaveBeenCalled(); - expect(onEndSpy).toHaveBeenCalled(); - - done(); - }, 0); - }, 0); - - }, 0); + expect(onHeadersSpy).toHaveBeenCalled(); + expect(onMessageSpy).not.toHaveBeenCalled(); + expect(onEndSpy).not.toHaveBeenCalled(); + + transport.sendMessages(); + expect(onHeadersSpy).toHaveBeenCalled(); + expect(onMessageSpy).toHaveBeenCalled(); + expect(onEndSpy).not.toHaveBeenCalled(); + + transport.sendTrailers(); + expect(onHeadersSpy).toHaveBeenCalled(); + expect(onMessageSpy).toHaveBeenCalled(); + expect(onEndSpy).toHaveBeenCalled(); }); }); @@ -352,4 +341,4 @@ describe("FakeTransportBuilder", () => { } client.finishSend(); } -}); \ No newline at end of file +}); diff --git a/client/grpc-web/src/client.ts b/client/grpc-web/src/client.ts index 15a54510..7676d06d 100644 --- a/client/grpc-web/src/client.ts +++ b/client/grpc-web/src/client.ts @@ -2,7 +2,6 @@ import {Metadata} from "./metadata"; import {ChunkParser, Chunk, ChunkType} from "./ChunkParser"; import {Code, httpStatusToCode} from "./Code"; import {debug} from "./debug"; -import detach from "./detach"; import {Transport, TransportFactory, makeDefaultTransport} from "./transports/Transport"; import {MethodDefinition} from "./service"; import {frameRequest} from "./util"; @@ -202,10 +201,8 @@ class GrpcClient { - detach(() => { - if (this.closed) return; - callback(code, message, trailers); - }); + if (this.closed) return; + callback(code, message, trailers); }); } @@ -213,9 +210,7 @@ class GrpcClient { - detach(() => { - callback(headers); - }); + callback(headers); }); } @@ -224,10 +219,8 @@ class GrpcClient { - detach(() => { - if (this.closed) return; - callback(code, msg, trailers); - }); + if (this.closed) return; + callback(code, msg, trailers); }); } @@ -235,10 +228,8 @@ class GrpcClient { - detach(() => { - if (this.closed) return; - callback(res); - }); + if (this.closed) return; + callback(res); }); } diff --git a/client/grpc-web/src/detach.ts b/client/grpc-web/src/detach.ts index 1513302d..ef6a5d04 100644 --- a/client/grpc-web/src/detach.ts +++ b/client/grpc-web/src/detach.ts @@ -1,43 +1,3 @@ -// awaitingExecution is null if there is no current timer -let awaitingExecution: Array<() => void> | null = null; - -function runCallbacks() { - if (awaitingExecution) { - // Use a new reference to the awaitingExecution array to allow callbacks to add to the "new" awaitingExecution array - const thisCallbackSet = awaitingExecution; - awaitingExecution = null; - for (let i = 0; i < thisCallbackSet.length; i++) { - try { - thisCallbackSet[i](); - } catch (e) { - if (awaitingExecution === null) { - awaitingExecution = []; - setTimeout(() => { - runCallbacks(); - }, 0); - } - // Add the remaining callbacks to the array so that they can be invoked on the next pass - for (let k = thisCallbackSet.length - 1; k > i; k--) { - awaitingExecution.unshift(thisCallbackSet[k]); - } - // rethrow the error - throw e; - } - } - } -} - -// detach executes the callbacks in the order they are added with no context - this is used to avoid errors thrown -// in user callbacks being caught by handlers such as fetch's catch. This function is necessary as setTimeout in -// Safari is prone to switching the order of execution of setTimeout(0). export default function detach(cb: () => void) { - if (awaitingExecution !== null) { - // there is a timer running, add to the list and this function will be executed with that existing timer - awaitingExecution.push(cb); - return; - } - awaitingExecution = [cb]; - setTimeout(() => { - runCallbacks(); - }, 0); + cb(); } diff --git a/client/grpc-web/src/transports/http/fetch.ts b/client/grpc-web/src/transports/http/fetch.ts index 29c76b0e..1dfc3e0b 100644 --- a/client/grpc-web/src/transports/http/fetch.ts +++ b/client/grpc-web/src/transports/http/fetch.ts @@ -1,7 +1,6 @@ import {Metadata} from "../../metadata"; import {Transport, TransportFactory, TransportOptions} from "../Transport"; import {debug} from "../../debug"; -import detach from "../../detach"; type Omit = Pick> export type FetchTransportInit = Omit; @@ -44,14 +43,10 @@ class Fetch implements Transport { this.reader.read() .then((result: { done: boolean, value: Uint8Array }) => { if (result.done) { - detach(() => { - this.options.onEnd(); - }); + this.options.onEnd(); return res; } - detach(() => { - this.options.onChunk(result.value); - }); + this.options.onChunk(result.value); this.pump(this.reader, res); return; }) @@ -62,9 +57,7 @@ class Fetch implements Transport { } this.cancelled = true; this.options.debug && debug("Fetch.catch", err.message); - detach(() => { - this.options.onEnd(err); - }); + this.options.onEnd(err); }); } @@ -77,9 +70,7 @@ class Fetch implements Transport { signal: this.controller && this.controller.signal }).then((res: Response) => { this.options.debug && debug("Fetch.response", res); - detach(() => { - this.options.onHeaders(new Metadata(res.headers as any), res.status); - }); + this.options.onHeaders(new Metadata(res.headers as any), res.status); if (res.body) { this.pump(res.body.getReader(), res) return; @@ -92,9 +83,7 @@ class Fetch implements Transport { } this.cancelled = true; this.options.debug && debug("Fetch.catch", err.message); - detach(() => { - this.options.onEnd(err); - }); + this.options.onEnd(err); }); } diff --git a/client/grpc-web/src/transports/http/xhr.ts b/client/grpc-web/src/transports/http/xhr.ts index 2f82a689..80756a96 100644 --- a/client/grpc-web/src/transports/http/xhr.ts +++ b/client/grpc-web/src/transports/http/xhr.ts @@ -1,7 +1,6 @@ import {Metadata} from "../../metadata"; import {Transport, TransportFactory, TransportOptions} from "../Transport"; import {debug} from "../../debug"; -import detach from "../../detach"; import {detectMozXHRSupport, detectXHROverrideMimeTypeSupport} from "./xhrUtil"; export interface XhrTransportInit { @@ -37,24 +36,18 @@ export class XHR implements Transport { const rawText = this.xhr.response.substr(this.index); this.index = this.xhr.response.length; const asArrayBuffer = stringToArrayBuffer(rawText); - detach(() => { - this.options.onChunk(asArrayBuffer); - }); + this.options.onChunk(asArrayBuffer); } onLoadEvent() { this.options.debug && debug("XHR.onLoadEvent"); - detach(() => { - this.options.onEnd(); - }); + this.options.onEnd(); } onStateChange() { this.options.debug && debug("XHR.onStateChange", this.xhr.readyState); if (this.xhr.readyState === XMLHttpRequest.HEADERS_RECEIVED) { - detach(() => { - this.options.onHeaders(new Metadata(this.xhr.getAllResponseHeaders()), this.xhr.status); - }); + this.options.onHeaders(new Metadata(this.xhr.getAllResponseHeaders()), this.xhr.status); } } @@ -85,9 +78,7 @@ export class XHR implements Transport { xhr.addEventListener("loadend", this.onLoadEvent.bind(this)); xhr.addEventListener("error", (err: ErrorEvent) => { this.options.debug && debug("XHR.error", err); - detach(() => { - this.options.onEnd(err.error); - }); + this.options.onEnd(err.error); }); } @@ -114,9 +105,7 @@ export class MozChunkedArrayBufferXHR extends XHR { onProgressEvent() { const resp = this.xhr.response; this.options.debug && debug("MozXHR.onProgressEvent: ", new Uint8Array(resp)); - detach(() => { - this.options.onChunk(new Uint8Array(resp)); - }); + this.options.onChunk(new Uint8Array(resp)); } } diff --git a/client/grpc-web/src/transports/websocket/websocket.ts b/client/grpc-web/src/transports/websocket/websocket.ts index 8bb8b389..ff1c8306 100644 --- a/client/grpc-web/src/transports/websocket/websocket.ts +++ b/client/grpc-web/src/transports/websocket/websocket.ts @@ -1,7 +1,6 @@ import {Metadata} from "../../metadata"; import {Transport, TransportFactory, TransportOptions} from "../Transport"; import {debug} from "../../debug"; -import detach from "../../detach"; import {encodeASCII} from "../../ChunkParser"; enum WebsocketSignal { @@ -68,9 +67,7 @@ function websocketRequest(options: TransportOptions): Transport { ws.onclose = function (closeEvent) { options.debug && debug("websocketRequest.onclose", closeEvent); - detach(() => { - options.onEnd(); - }); + options.onEnd(); }; ws.onerror = function (error) { @@ -78,17 +75,13 @@ function websocketRequest(options: TransportOptions): Transport { }; ws.onmessage = function (e) { - detach(() => { - options.onChunk(new Uint8Array(e.data)); - }); + options.onChunk(new Uint8Array(e.data)); }; }, cancel: () => { options.debug && debug("websocket.abort"); - detach(() => { - ws.close(); - }); + ws.close(); } }; } diff --git a/integration_test/ts/src/detach.spec.ts b/integration_test/ts/src/detach.spec.ts deleted file mode 100644 index 2c3b4f75..00000000 --- a/integration_test/ts/src/detach.spec.ts +++ /dev/null @@ -1,131 +0,0 @@ -import {assert} from "chai"; -import detach from "../../../client/grpc-web/src/detach"; -import {UncaughtExceptionListener} from "./util"; -import { conditionallyRunTestSuite, SuiteEnum } from "../suiteUtils"; - -conditionallyRunTestSuite(SuiteEnum.detach, () => { - describe("basic execution ordering", () => { - it("should invoke a function", (done) => { - detach(() => { - done(); - }); - }); - - it("should invoke multiple functions in the order they are added", (done) => { - let index = 0; - detach(() => { - assert.equal(index, 0); - index++; - }); - detach(() => { - assert.equal(index, 1); - index++; - }); - detach(() => { - assert.equal(index, 2); - index++; - done(); - }); - }); - - it("should invoke multiple functions in the order they are added after the current context", (done) => { - let index = 0; - detach(() => { - assert.equal(index, 5); - index++; - }); - detach(() => { - assert.equal(index, 6); - index++; - }); - detach(() => { - assert.equal(index, 7); - index++; - done(); - }); - index = 5; // This should be run before the first callback - }); - }); - - describe("exception handling", () => { - let uncaughtHandler: UncaughtExceptionListener; - beforeEach(() => { - uncaughtHandler = new UncaughtExceptionListener(); - uncaughtHandler.attach(); - }); - - afterEach(() => { - uncaughtHandler.detach(); - }); - - it("should invoke remaining function when exceptions are thrown", (done) => { - let index = 0; - detach(() => { - assert.equal(index, 0); - index++; - }); - detach(() => { - assert.equal(index, 1); - index++; - throw new Error("Second callback threw error"); - }); - detach(() => { - assert.equal(index, 2); - index++; - throw new Error("Third callback threw error"); - }); - detach(() => { - uncaughtHandler.detach(); - assert.equal(index, 3); - index++; - const exceptionsCaught = uncaughtHandler.getMessages(); - assert.lengthOf(exceptionsCaught, 2); - assert.include(exceptionsCaught[0], "Second callback threw error"); - assert.include(exceptionsCaught[1], "Third callback threw error"); - done(); - }); - }); - - // This test is weakly-held. It is here to detect changes to execution ordering with other contexts, but detach - // should not be used in a manner that relies on this behaviour. - it("should invoke remaining function when exceptions are thrown", (done) => { - let index = 0; - detach(() => { - assert.equal(index, 0); - index++; - }); - detach(() => { - assert.equal(index, 1); - index++; - throw new Error("Second callback threw error"); - }); - // setTimeout below will be called between the above and below functions - detach(() => { - assert.equal(index, 3); - index++; - setTimeout(() => { - assert.equal(index, 4); - index++; - }, 0); - throw new Error("Third callback threw error"); - }); - detach(() => { - uncaughtHandler.detach(); - assert.equal(index, 5); - index++; - const exceptionsCaught = uncaughtHandler.getMessages(); - assert.lengthOf(exceptionsCaught, 2); - assert.include(exceptionsCaught[0], "Second callback threw error"); - assert.include(exceptionsCaught[1], "Third callback threw error"); - done(); - }); - - // This function will be called after the first exception is thrown. Throwing exceptions causes the subsequent - // callbacks to be run in a separate, subsequent timeout. - setTimeout(() => { - assert.equal(index, 2); - index++; - }, 0); - }); - }); -}); \ No newline at end of file diff --git a/integration_test/ts/src/spec.ts b/integration_test/ts/src/spec.ts index b3efacc5..00f45a93 100644 --- a/integration_test/ts/src/spec.ts +++ b/integration_test/ts/src/spec.ts @@ -3,7 +3,6 @@ import "./client.websocket.spec"; import "./invoke.spec"; import "./unary.spec"; import "./cancellation.spec"; -import "./detach.spec"; import "./ChunkParser.spec"; -jasmine.DEFAULT_TIMEOUT_INTERVAL = 10000; \ No newline at end of file +jasmine.DEFAULT_TIMEOUT_INTERVAL = 10000; From 1ac9eb34501b09e799e32b383cd37dbf0c4c02d6 Mon Sep 17 00:00:00 2001 From: Marcus Longmuir Date: Fri, 11 Oct 2019 13:45:09 +0100 Subject: [PATCH 2/7] Included setTimeouts in tests --- .../grpc-web-fake-transport/src/index.spec.ts | 36 +++++++++++-------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/client/grpc-web-fake-transport/src/index.spec.ts b/client/grpc-web-fake-transport/src/index.spec.ts index 8a80ef56..70ccf247 100644 --- a/client/grpc-web-fake-transport/src/index.spec.ts +++ b/client/grpc-web-fake-transport/src/index.spec.ts @@ -227,7 +227,7 @@ describe("FakeTransportBuilder", () => { }); describe("manual trigger", () => { - it("should allow the consumer to control the lifecycle of the server response", () => { + it("should allow the consumer to control the lifecycle of the server response", done => { const onHeadersSpy = jest.fn(); const onMessageSpy = jest.fn(); const onEndSpy = jest.fn(); @@ -258,19 +258,27 @@ describe("FakeTransportBuilder", () => { transport.sendHeaders(); - expect(onHeadersSpy).toHaveBeenCalled(); - expect(onMessageSpy).not.toHaveBeenCalled(); - expect(onEndSpy).not.toHaveBeenCalled(); - - transport.sendMessages(); - expect(onHeadersSpy).toHaveBeenCalled(); - expect(onMessageSpy).toHaveBeenCalled(); - expect(onEndSpy).not.toHaveBeenCalled(); - - transport.sendTrailers(); - expect(onHeadersSpy).toHaveBeenCalled(); - expect(onMessageSpy).toHaveBeenCalled(); - expect(onEndSpy).toHaveBeenCalled(); + setTimeout(() => { + expect(onHeadersSpy).toHaveBeenCalled(); + expect(onMessageSpy).not.toHaveBeenCalled(); + expect(onEndSpy).not.toHaveBeenCalled(); + + transport.sendMessages(); + setTimeout(() => { + expect(onHeadersSpy).toHaveBeenCalled(); + expect(onMessageSpy).toHaveBeenCalled(); + expect(onEndSpy).not.toHaveBeenCalled(); + + transport.sendTrailers(); + setTimeout(() => { + expect(onHeadersSpy).toHaveBeenCalled(); + expect(onMessageSpy).toHaveBeenCalled(); + expect(onEndSpy).toHaveBeenCalled(); + + done(); + }, 0); + }, 0); + }, 0); }); }); From 7cb021b426617cca1003745e1597ef9b62caed97 Mon Sep 17 00:00:00 2001 From: Marcus Longmuir Date: Fri, 11 Oct 2019 16:12:59 +0100 Subject: [PATCH 3/7] Fixed fake transport race --- client/grpc-web-fake-transport/src/index.spec.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/client/grpc-web-fake-transport/src/index.spec.ts b/client/grpc-web-fake-transport/src/index.spec.ts index 70ccf247..f0125fbe 100644 --- a/client/grpc-web-fake-transport/src/index.spec.ts +++ b/client/grpc-web-fake-transport/src/index.spec.ts @@ -8,6 +8,7 @@ describe("FakeTransportBuilder", () => { it("should allow the response messages to be stubbed", done => { const pingResponseMsg = makePingResponse("hello"); const transport = new FakeTransportBuilder() + .withManualTrigger() .withMessages([ pingResponseMsg ]) .build(); @@ -20,6 +21,7 @@ describe("FakeTransportBuilder", () => { it("should allow the response trailers to be stubbed", done => { const expectedTrailers = new grpc.Metadata({ foo: "bar" }); const transport = new FakeTransportBuilder() + .withManualTrigger() .withTrailers(expectedTrailers) .build(); @@ -31,6 +33,7 @@ describe("FakeTransportBuilder", () => { it("should allow an error to be injected before any headers are sent", done => { const transport = new FakeTransportBuilder() + .withManualTrigger() .withPreHeadersError(500) .withMessages([ makePingResponse("hello") ]) .build(); @@ -50,6 +53,7 @@ describe("FakeTransportBuilder", () => { const pingResponseMsg = makePingResponse("hello"); const trailers = new grpc.Metadata({ foo: "bar" }); const transport = new FakeTransportBuilder() + .withManualTrigger() .withPreMessagesError(grpc.Code.FailedPrecondition, "failed precondition :)") .withMessages([ pingResponseMsg ]) .withTrailers(trailers) @@ -75,6 +79,7 @@ describe("FakeTransportBuilder", () => { const pingResponseMsg = makePingResponse("hello"); const trailers = new grpc.Metadata({ foo: "bar" }); const transport = new FakeTransportBuilder() + .withManualTrigger() .withPreTrailersError(grpc.Code.FailedPrecondition, "failed precondition :)") .withMessages([ pingResponseMsg ]) .withTrailers(trailers) @@ -100,6 +105,7 @@ describe("FakeTransportBuilder", () => { it("should not be called if no message is sent", done => { const messageSpy = jest.fn(); const transport = new FakeTransportBuilder() + .withManualTrigger() .withMessageListener(messageSpy) .build(); @@ -115,6 +121,7 @@ describe("FakeTransportBuilder", () => { const expectedBytes = frameRequest(req); const transport = new FakeTransportBuilder() + .withManualTrigger() .withMessageListener(messageSpy) .build(); @@ -132,6 +139,7 @@ describe("FakeTransportBuilder", () => { const expectedBytes = [ frameRequest(reqA), frameRequest(reqB) ]; const transport = new FakeTransportBuilder() + .withManualTrigger() .withMessageListener(messageSpy) .build(); @@ -151,6 +159,7 @@ describe("FakeTransportBuilder", () => { const expectedHeaders = new grpc.Metadata({ expected: "header" }); const transport = new FakeTransportBuilder() + .withManualTrigger() .withHeadersListener(headersSpy) .build(); @@ -163,6 +172,7 @@ describe("FakeTransportBuilder", () => { client.start(expectedHeaders); client.finishSend(); + transport.sendAll(); }) }); @@ -171,6 +181,7 @@ describe("FakeTransportBuilder", () => { const requestSpy = jest.fn(); const transport = new FakeTransportBuilder() + .withManualTrigger() .withRequestListener(requestSpy) .build(); @@ -189,6 +200,7 @@ describe("FakeTransportBuilder", () => { const cancelSpy = jest.fn(); const transport = new FakeTransportBuilder() + .withManualTrigger() .withCancelListener(cancelSpy) .build(); @@ -208,6 +220,7 @@ describe("FakeTransportBuilder", () => { const finishSendSpy = jest.fn(); const transport = new FakeTransportBuilder() + .withManualTrigger() .withFinishSendListener(finishSendSpy) .build(); @@ -233,6 +246,7 @@ describe("FakeTransportBuilder", () => { const onEndSpy = jest.fn(); const transport = new FakeTransportBuilder() + .withManualTrigger() .withManualTrigger() .withHeaders(new grpc.Metadata({ header: "value" })) .withMessages([ makePingResponse("msgA") ]) @@ -339,6 +353,7 @@ describe("FakeTransportBuilder", () => { client.start(); client.send(req); client.finishSend(); + transport.sendAll(); } function doPingStreamRequest(transport: TriggerableTransport, requests: PingRequest[], callback: (data: RequestResponse) => void) { @@ -348,5 +363,6 @@ describe("FakeTransportBuilder", () => { client.send(req); } client.finishSend(); + transport.sendAll(); } }); From e1e10d7def778e2bc84a1d46f75d34b8141a9884 Mon Sep 17 00:00:00 2001 From: Marcus Longmuir Date: Sun, 13 Oct 2019 19:44:00 +0100 Subject: [PATCH 4/7] Rethrowing callback exceptions --- client/grpc-web/src/client.ts | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/client/grpc-web/src/client.ts b/client/grpc-web/src/client.ts index 7676d06d..1f53e871 100644 --- a/client/grpc-web/src/client.ts +++ b/client/grpc-web/src/client.ts @@ -202,7 +202,13 @@ class GrpcClient { if (this.closed) return; - callback(code, message, trailers); + try { + callback(code, message, trailers); + } catch (e) { + setTimeout(() => { + throw e; + }) + } }); } @@ -210,7 +216,13 @@ class GrpcClient { - callback(headers); + try { + callback(headers); + } catch (e) { + setTimeout(() => { + throw e; + }) + } }); } @@ -220,7 +232,13 @@ class GrpcClient { if (this.closed) return; - callback(code, msg, trailers); + try { + callback(code, msg, trailers); + } catch (e) { + setTimeout(() => { + throw e; + }) + } }); } @@ -229,7 +247,13 @@ class GrpcClient { if (this.closed) return; - callback(res); + try { + callback(res); + } catch (e) { + setTimeout(() => { + throw e; + }) + } }); } From 92f78738e27ef7f29434a4eab3803544f418fefc Mon Sep 17 00:00:00 2001 From: Marcus Longmuir Date: Wed, 6 Nov 2019 11:50:51 +0000 Subject: [PATCH 5/7] Removed detach suite --- integration_test/ts/suiteUtils.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/integration_test/ts/suiteUtils.ts b/integration_test/ts/suiteUtils.ts index 792830de..6339defd 100644 --- a/integration_test/ts/suiteUtils.ts +++ b/integration_test/ts/suiteUtils.ts @@ -5,7 +5,6 @@ export enum SuiteEnum { unary, ChunkParser, cancellation, - detach, } type enumMap = {[key: string]: number} & {[key: number]: string}; From 2f66ce96294221d1f1d7110657d67acbfe1fd2c5 Mon Sep 17 00:00:00 2001 From: Marcus Longmuir Date: Wed, 6 Nov 2019 14:49:46 +0000 Subject: [PATCH 6/7] Simplify FakeTransport changes and change autoTrigger behaviour --- .../grpc-web-fake-transport/src/index.spec.ts | 56 ++++++------------- client/grpc-web-fake-transport/src/index.ts | 8 +-- 2 files changed, 21 insertions(+), 43 deletions(-) diff --git a/client/grpc-web-fake-transport/src/index.spec.ts b/client/grpc-web-fake-transport/src/index.spec.ts index f0125fbe..a8e102dd 100644 --- a/client/grpc-web-fake-transport/src/index.spec.ts +++ b/client/grpc-web-fake-transport/src/index.spec.ts @@ -8,7 +8,6 @@ describe("FakeTransportBuilder", () => { it("should allow the response messages to be stubbed", done => { const pingResponseMsg = makePingResponse("hello"); const transport = new FakeTransportBuilder() - .withManualTrigger() .withMessages([ pingResponseMsg ]) .build(); @@ -21,7 +20,6 @@ describe("FakeTransportBuilder", () => { it("should allow the response trailers to be stubbed", done => { const expectedTrailers = new grpc.Metadata({ foo: "bar" }); const transport = new FakeTransportBuilder() - .withManualTrigger() .withTrailers(expectedTrailers) .build(); @@ -33,7 +31,6 @@ describe("FakeTransportBuilder", () => { it("should allow an error to be injected before any headers are sent", done => { const transport = new FakeTransportBuilder() - .withManualTrigger() .withPreHeadersError(500) .withMessages([ makePingResponse("hello") ]) .build(); @@ -53,7 +50,6 @@ describe("FakeTransportBuilder", () => { const pingResponseMsg = makePingResponse("hello"); const trailers = new grpc.Metadata({ foo: "bar" }); const transport = new FakeTransportBuilder() - .withManualTrigger() .withPreMessagesError(grpc.Code.FailedPrecondition, "failed precondition :)") .withMessages([ pingResponseMsg ]) .withTrailers(trailers) @@ -79,7 +75,6 @@ describe("FakeTransportBuilder", () => { const pingResponseMsg = makePingResponse("hello"); const trailers = new grpc.Metadata({ foo: "bar" }); const transport = new FakeTransportBuilder() - .withManualTrigger() .withPreTrailersError(grpc.Code.FailedPrecondition, "failed precondition :)") .withMessages([ pingResponseMsg ]) .withTrailers(trailers) @@ -105,7 +100,6 @@ describe("FakeTransportBuilder", () => { it("should not be called if no message is sent", done => { const messageSpy = jest.fn(); const transport = new FakeTransportBuilder() - .withManualTrigger() .withMessageListener(messageSpy) .build(); @@ -121,8 +115,9 @@ describe("FakeTransportBuilder", () => { const expectedBytes = frameRequest(req); const transport = new FakeTransportBuilder() - .withManualTrigger() - .withMessageListener(messageSpy) + .withMessageListener(messageBytes => { + messageSpy(messageBytes); + }) .build(); doPingStreamRequest(transport, [ req ], () => { @@ -139,7 +134,6 @@ describe("FakeTransportBuilder", () => { const expectedBytes = [ frameRequest(reqA), frameRequest(reqB) ]; const transport = new FakeTransportBuilder() - .withManualTrigger() .withMessageListener(messageSpy) .build(); @@ -159,7 +153,6 @@ describe("FakeTransportBuilder", () => { const expectedHeaders = new grpc.Metadata({ expected: "header" }); const transport = new FakeTransportBuilder() - .withManualTrigger() .withHeadersListener(headersSpy) .build(); @@ -172,7 +165,6 @@ describe("FakeTransportBuilder", () => { client.start(expectedHeaders); client.finishSend(); - transport.sendAll(); }) }); @@ -181,7 +173,6 @@ describe("FakeTransportBuilder", () => { const requestSpy = jest.fn(); const transport = new FakeTransportBuilder() - .withManualTrigger() .withRequestListener(requestSpy) .build(); @@ -200,7 +191,6 @@ describe("FakeTransportBuilder", () => { const cancelSpy = jest.fn(); const transport = new FakeTransportBuilder() - .withManualTrigger() .withCancelListener(cancelSpy) .build(); @@ -220,7 +210,6 @@ describe("FakeTransportBuilder", () => { const finishSendSpy = jest.fn(); const transport = new FakeTransportBuilder() - .withManualTrigger() .withFinishSendListener(finishSendSpy) .build(); @@ -240,13 +229,12 @@ describe("FakeTransportBuilder", () => { }); describe("manual trigger", () => { - it("should allow the consumer to control the lifecycle of the server response", done => { + it("should allow the consumer to control the lifecycle of the server response", () => { const onHeadersSpy = jest.fn(); const onMessageSpy = jest.fn(); const onEndSpy = jest.fn(); const transport = new FakeTransportBuilder() - .withManualTrigger() .withManualTrigger() .withHeaders(new grpc.Metadata({ header: "value" })) .withMessages([ makePingResponse("msgA") ]) @@ -272,27 +260,19 @@ describe("FakeTransportBuilder", () => { transport.sendHeaders(); - setTimeout(() => { - expect(onHeadersSpy).toHaveBeenCalled(); - expect(onMessageSpy).not.toHaveBeenCalled(); - expect(onEndSpy).not.toHaveBeenCalled(); - - transport.sendMessages(); - setTimeout(() => { - expect(onHeadersSpy).toHaveBeenCalled(); - expect(onMessageSpy).toHaveBeenCalled(); - expect(onEndSpy).not.toHaveBeenCalled(); - - transport.sendTrailers(); - setTimeout(() => { - expect(onHeadersSpy).toHaveBeenCalled(); - expect(onMessageSpy).toHaveBeenCalled(); - expect(onEndSpy).toHaveBeenCalled(); - - done(); - }, 0); - }, 0); - }, 0); + expect(onHeadersSpy).toHaveBeenCalled(); + expect(onMessageSpy).not.toHaveBeenCalled(); + expect(onEndSpy).not.toHaveBeenCalled(); + + transport.sendMessages(); + expect(onHeadersSpy).toHaveBeenCalled(); + expect(onMessageSpy).toHaveBeenCalled(); + expect(onEndSpy).not.toHaveBeenCalled(); + + transport.sendTrailers(); + expect(onHeadersSpy).toHaveBeenCalled(); + expect(onMessageSpy).toHaveBeenCalled(); + expect(onEndSpy).toHaveBeenCalled(); }); }); @@ -353,7 +333,6 @@ describe("FakeTransportBuilder", () => { client.start(); client.send(req); client.finishSend(); - transport.sendAll(); } function doPingStreamRequest(transport: TriggerableTransport, requests: PingRequest[], callback: (data: RequestResponse) => void) { @@ -363,6 +342,5 @@ describe("FakeTransportBuilder", () => { client.send(req); } client.finishSend(); - transport.sendAll(); } }); diff --git a/client/grpc-web-fake-transport/src/index.ts b/client/grpc-web-fake-transport/src/index.ts index 1fcac86a..d41525e5 100644 --- a/client/grpc-web-fake-transport/src/index.ts +++ b/client/grpc-web-fake-transport/src/index.ts @@ -277,9 +277,6 @@ export class FakeTransportBuilder { if (mock.headersListener) { mock.headersListener(metadata); } - if (mock.autoTrigger) { - triggers.sendAll(); - } }, sendMessage: (msgBytes: ArrayBufferView) => { if (mock.messageListener) { @@ -290,6 +287,9 @@ export class FakeTransportBuilder { if (mock.finishSendListener) { mock.finishSendListener(); } + if (mock.autoTrigger) { + triggers.sendAll(); + } }, cancel: () => { if (mock.cancelListener) { @@ -308,4 +308,4 @@ export interface TriggerableTransport extends grpc.TransportFactory { sendMessages(): boolean; sendTrailers(): boolean; sendAll(): void; -} \ No newline at end of file +} From 45373d19ee559e66cc8b7499162afbb920df5ed8 Mon Sep 17 00:00:00 2001 From: Marcus Longmuir Date: Wed, 6 Nov 2019 15:41:18 +0000 Subject: [PATCH 7/7] Added README explanation of withManualTrigger() --- client/grpc-web-fake-transport/README.md | 34 +++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/client/grpc-web-fake-transport/README.md b/client/grpc-web-fake-transport/README.md index 5069b956..2950b8c8 100644 --- a/client/grpc-web-fake-transport/README.md +++ b/client/grpc-web-fake-transport/README.md @@ -3,6 +3,14 @@ ## Usage +`FakeTransportBuilder` builds a `Transport` that can be configured to send preset headers, messages, and trailers for testing. + +By default the `Transport` that `FakeTranportBuilder.build()` generates will trigger all of the specified response behaviours (`.withHeaders(metadata)`, `.withMessages([msgOne, msgTwo])`, `.withTrailers(metadata)`) when the client has finished sending. + +This is usually the desired flow as all of the response behaviour is triggered only after the client has finished sending as would most commonly occur in production usage. + +However, in the case of bi-directional or other complex usage it can be helpful to use `.withManualTrigger()` to disable automatic sending of messages or headers/trailers and trigger the sending manually using `sendHeaders()`, `sendMessages()` and `sendTrailers()`. + ### With Service Stubs generated by ts-protoc-gen ```typescript import { FakeTransportBuilder } from '@improbable-eng/grpc-web-fake-transport'; @@ -34,6 +42,30 @@ grpc.invoke(PingService.DoPing, { }); ``` +### With `withManualTrigger()` +```typescript +import { grpc } from '@improbable-eng/grpc-web'; +import { FakeTransportBuilder } from '@improbable-eng/grpc-web-fake-transport'; + +const fakeTransport = new FakeTransportBuilder() + .withManualTrigger() + .withHeaders(new grpc.Metadata({ headerKey: "value" })) + .withMessages([ new PingResponse() ]) + .withTrailers(new grpc.Metadata({ trailerKey: "value" })) + .build(); + +grpc.invoke(PingService.DoPing, { + host: "https://example.com", + transport: fakeTransport, + /* ... */ +}); + +// Manually trigger the response behaviours +fakeTransport.sendHeaders(); +fakeTransport.sendMessages(); +fakeTransport.sendTrailers(); +``` + Alternatively replace the Default Transport when initialising your tests: ```typescript import { grpc } from "@improbable-eng/grpc-web"; @@ -41,4 +73,4 @@ import { NodeHttpTransport } from "@improbable-eng/grpc-web-fake-transport"; // Do this first, before you make any grpc requests! grpc.setDefaultTransport(fakeTransport); -``` \ No newline at end of file +```