From a37c271469e61df6beaf3901a48fbbee4b9b45db Mon Sep 17 00:00:00 2001 From: Emiliano Quiroga Date: Tue, 13 Jun 2023 16:46:36 -0300 Subject: [PATCH 1/3] fix tests updated NamedPipeTransport class to handle null and destroyed sockets updated NamedPipe tests --- .../src/namedPipe/namedPipeTransport.ts | 6 +- .../tests/NamedPipe.test.js | 115 ++++++++++++------ 2 files changed, 84 insertions(+), 37 deletions(-) diff --git a/libraries/botframework-streaming/src/namedPipe/namedPipeTransport.ts b/libraries/botframework-streaming/src/namedPipe/namedPipeTransport.ts index 2daebe1306..56278cc23b 100644 --- a/libraries/botframework-streaming/src/namedPipe/namedPipeTransport.ts +++ b/libraries/botframework-streaming/src/namedPipe/namedPipeTransport.ts @@ -85,7 +85,11 @@ export class NamedPipeTransport implements ITransportSender, ITransportReceiver * @returns The buffer containing the data from the transport. */ receive(count: number): Promise { - if (this._activeReceiveResolve) { + if (!this.socket) { + throw new Error('Socket unavailable to make the connection.'); + } else if (this.socket.destroyed) { + throw new Error('Socket was destroyed.'); + } else if (this._activeReceiveResolve) { throw new Error('Cannot call receive more than once before it has returned.'); } diff --git a/libraries/botframework-streaming/tests/NamedPipe.test.js b/libraries/botframework-streaming/tests/NamedPipe.test.js index 5892db99b2..9c756ca748 100644 --- a/libraries/botframework-streaming/tests/NamedPipe.test.js +++ b/libraries/botframework-streaming/tests/NamedPipe.test.js @@ -1,12 +1,10 @@ const assert = require('assert'); const { expect } = require('chai'); -const { expectEventually } = require('./helpers/expectEventually'); -const { NamedPipeClient, NamedPipeServer, StreamingRequest } = require('../lib'); +const { NamedPipeClient, NamedPipeServer, StreamingRequest, StreamingResponse } = require('../lib'); const { NamedPipeTransport } = require('../lib/namedPipe'); const { platform } = require('os'); const { RequestHandler } = require('../lib'); const { createNodeServer, getServerFactory } = require('../lib/utilities/createNodeServer'); - class FauxSock { constructor(contentString) { if (contentString) { @@ -166,9 +164,7 @@ describe.windowsOnly('Streaming Extensions NamedPipe Library Tests', function () expect(() => transport.close()).to.not.throw(); }); - // TODO: 2023-04-24 [hawo] #4462 The code today does not allows the receive() call to be rejected by reading a dead socket. - // The receive() call will be rejected IFF the socket is closed/error AFTER the receive() call. - it.skip('throws when reading from a dead socket', async function () { + it('throws when reading from a dead socket', async function () { const sock = new FauxSock(); sock.destroyed = true; sock.connecting = false; @@ -176,7 +172,7 @@ describe.windowsOnly('Streaming Extensions NamedPipe Library Tests', function () const transport = new NamedPipeTransport(sock, 'fakeSocket5'); expect(transport).to.be.instanceOf(NamedPipeTransport); expect(transport.isConnected).to.be.false; - (await expectEventually(transport.receive(5))).to.throw(); + expect(() => transport.receive(5)).to.throw(); expect(() => transport.close()).to.not.throw(); }); @@ -258,20 +254,44 @@ describe.windowsOnly('Streaming Extensions NamedPipe Library Tests', function () expect(() => client.disconnect()).to.not.throw(); }); - // TODO: 2023-04-24 [hawo] #4462 The client.send() call will only resolve when the other side responded. - // Because the other side is not connected to anything, thus, no response is received. - // Thus, the Promise is not resolved. - it.skip('sends without throwing', function (done) { - const req = new StreamingRequest(); - req.Verb = 'POST'; - req.Path = 'some/path'; - req.setBody('Hello World!'); - client - .send(req) - .catch((err) => { - expect(err).to.be.undefined; - }) - .then(done); + it('sends without throwing', function (done) { + const response = new StreamingResponse(); + response.statusCode = 111; + response.setBody('Test body.'); + class TestRequestHandler extends RequestHandler { + constructor() { + super(); + } + processRequest(_request, _logger) { + return response; + } + } + const server = new NamedPipeServer('pipeClientTest', new TestRequestHandler()); + const client = new NamedPipeClient('pipeClientTest', false); + const pingServer = () => + new Promise((resolve) => { + // Ping server before sending any information, so the server makes the connection over the socket. + client.send(new StreamingRequest()); + const interval = setInterval(() => { + if (server.isConnected) { + clearInterval(interval); + resolve(); + } + }, 100); + }); + try { + server.start(async () => { + await client.connect(); + await pingServer(); + const result = await client.send(new StreamingRequest()); + expect(result.statusCode).to.equal(response.statusCode); + server.disconnect(); + client.disconnect(); + done(); + }); + } catch (err) { + done(err); + } }); }); @@ -310,21 +330,44 @@ describe.windowsOnly('Streaming Extensions NamedPipe Library Tests', function () expect(server.isConnected).to.be.true; }); - // TODO: 2023-04-24 [hawo] #4462 The client.send() call will only resolve when the other side responded. - // Because the other side is not connected to anything, thus, no response is received. - // Thus, the Promise is not resolved. - it.skip('sends without throwing', function (done) { - const server = new NamedPipeServer('pipeA', new RequestHandler()); - expect(server).to.be.instanceOf(NamedPipeServer); - expect(() => server.start()).to.not.throw(); - const req = { verb: 'POST', path: '/api/messages', streams: [] }; - server - .send(req) - .catch((err) => { - expect(err).to.be.undefined; - }) - .then(expect(() => server.disconnect()).to.not.throw()) - .then(done); + it('sends without throwing', function (done) { + const response = new StreamingResponse(); + response.statusCode = 111; + response.setBody('Test body.'); + class TestRequestHandler extends RequestHandler { + constructor() { + super(); + } + processRequest(_request, _logger) { + return response; + } + } + const server = new NamedPipeServer('pipeServerTest'); + const client = new NamedPipeClient('pipeServerTest', new TestRequestHandler()); + const pingServer = () => + new Promise((resolve) => { + // Ping server before sending any information, so the server makes the connection over the socket. + client.send(new StreamingRequest()); + const interval = setInterval(() => { + if (server.isConnected) { + clearInterval(interval); + resolve(); + } + }, 100); + }); + try { + server.start(async () => { + await client.connect(); + await pingServer(); + const result = await server.send(new StreamingRequest()); + expect(result.statusCode).to.equal(response.statusCode); + server.disconnect(); + client.disconnect(); + done(); + }); + } catch (err) { + done(err); + } }); it('handles being disconnected', function () { From 4e099885a8134fe37d8fe423457b5df0f4bc4ac2 Mon Sep 17 00:00:00 2001 From: Emiliano Quiroga Date: Tue, 13 Jun 2023 18:10:43 -0300 Subject: [PATCH 2/3] suggested changes applied --- .../src/namedPipe/namedPipeTransport.ts | 4 ++-- .../tests/NamedPipe.test.js | 15 ++++++++++++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/libraries/botframework-streaming/src/namedPipe/namedPipeTransport.ts b/libraries/botframework-streaming/src/namedPipe/namedPipeTransport.ts index 56278cc23b..69f0824786 100644 --- a/libraries/botframework-streaming/src/namedPipe/namedPipeTransport.ts +++ b/libraries/botframework-streaming/src/namedPipe/namedPipeTransport.ts @@ -86,9 +86,9 @@ export class NamedPipeTransport implements ITransportSender, ITransportReceiver */ receive(count: number): Promise { if (!this.socket) { - throw new Error('Socket unavailable to make the connection.'); + throw new Error('Cannot receive data over an unavailable/null socket.'); } else if (this.socket.destroyed) { - throw new Error('Socket was destroyed.'); + throw new Error('Cannot receive data over a dead/destroyed socket.'); } else if (this._activeReceiveResolve) { throw new Error('Cannot call receive more than once before it has returned.'); } diff --git a/libraries/botframework-streaming/tests/NamedPipe.test.js b/libraries/botframework-streaming/tests/NamedPipe.test.js index 9c756ca748..37c854d9a7 100644 --- a/libraries/botframework-streaming/tests/NamedPipe.test.js +++ b/libraries/botframework-streaming/tests/NamedPipe.test.js @@ -176,6 +176,19 @@ describe.windowsOnly('Streaming Extensions NamedPipe Library Tests', function () expect(() => transport.close()).to.not.throw(); }); + it('throws when reading from a unavailable/null socket', async function () { + const sock = new FauxSock(); + sock.destroyed = false; + sock.connecting = false; + sock.writable = true; + const transport = new NamedPipeTransport(sock, 'fakeSocket5'); + expect(transport).to.be.instanceOf(NamedPipeTransport); + expect(() => transport.close()).to.not.throw(); + expect(transport.isConnected).to.be.false; + expect(transport.socket).to.be.null; + expect(() => transport.receive(5)).to.throw(); + }); + it('can read from the socket', function () { const sock = new FauxSock(); sock.destroyed = false; @@ -267,7 +280,7 @@ describe.windowsOnly('Streaming Extensions NamedPipe Library Tests', function () } } const server = new NamedPipeServer('pipeClientTest', new TestRequestHandler()); - const client = new NamedPipeClient('pipeClientTest', false); + const client = new NamedPipeClient('pipeClientTest'); const pingServer = () => new Promise((resolve) => { // Ping server before sending any information, so the server makes the connection over the socket. From 1e624bfadceca4657d1a21e23f95c758809e870d Mon Sep 17 00:00:00 2001 From: Emiliano Quiroga Date: Fri, 16 Jun 2023 11:33:31 -0300 Subject: [PATCH 3/3] Add suggested changes --- libraries/botframework-streaming/tests/NamedPipe.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libraries/botframework-streaming/tests/NamedPipe.test.js b/libraries/botframework-streaming/tests/NamedPipe.test.js index 37c854d9a7..762b3c8dfd 100644 --- a/libraries/botframework-streaming/tests/NamedPipe.test.js +++ b/libraries/botframework-streaming/tests/NamedPipe.test.js @@ -172,7 +172,7 @@ describe.windowsOnly('Streaming Extensions NamedPipe Library Tests', function () const transport = new NamedPipeTransport(sock, 'fakeSocket5'); expect(transport).to.be.instanceOf(NamedPipeTransport); expect(transport.isConnected).to.be.false; - expect(() => transport.receive(5)).to.throw(); + expect(() => transport.receive(5)).to.throw('Cannot receive data over a dead/destroyed socket.'); expect(() => transport.close()).to.not.throw(); }); @@ -186,7 +186,7 @@ describe.windowsOnly('Streaming Extensions NamedPipe Library Tests', function () expect(() => transport.close()).to.not.throw(); expect(transport.isConnected).to.be.false; expect(transport.socket).to.be.null; - expect(() => transport.receive(5)).to.throw(); + expect(() => transport.receive(5)).to.throw('Cannot receive data over an unavailable/null socket.'); }); it('can read from the socket', function () {