Revised request / inject abort handling. Closes #4294 (#4295)

API.md

@@ -1946,6 +1946,9 @@ Return value: a response object with the following properties:
 
 - `request` - the [request object](#request).
 
+Throws a Boom error if the request processing fails. The partial response object is exposed on
+the `data` property.
+
 ```js
 const Hapi = require('@hapi/hapi');
 

lib/request.js

@@ -363,9 +363,7 @@ exports = module.exports = internals.Request = class {
     async _lifecycle() {
 
         for (const func of this._route._cycle) {
-            if (this._isReplied ||
-                !this._eventContext.request) {
-
+            if (this._isReplied) {
                 return;
             }
 
@@ -433,15 +431,15 @@ exports = module.exports = internals.Request = class {
             clearTimeout(this._serverTimeoutId);
         }
 
+        if (exit) {                                                     // Can be a valid response or error (if returned from an ext, already handled because this.response is also set)
+            this._setResponse(this._core.Response.wrap(exit, this));    // Wrap to ensure any object thrown is always a valid Boom or Response object
+        }
+
         if (!this._eventContext.request) {
             this._finalize();
             return;
         }
 
-        if (exit) {                                                     // Can be a valid response or error (if returned from an ext, already handled because this.response is also set)
-            this._setResponse(this._core.Response.wrap(exit, this));    // Wrap to ensure any object thrown is always a valid Boom or Response object
-        }
-
         if (typeof this.response === 'symbol') {                        // close or abandon
             this._abort();
             return;
@@ -725,7 +723,12 @@ internals.event = function ({ request }, event, err) {
     request._eventContext.request = null;
 
     if (event === 'abort') {
-        request._setResponse(new Boom.Boom('Request aborted', { statusCode: request.route.settings.response.disconnectStatusCode }));
+
+        // Calling _reply() means that the abort is applied immediately, unless the response has already
+        // called _reply(), in which case this call is ignored and the transmit logic is responsible for
+        // handling the abort.
+
+        request._reply(new Boom.Boom('Request aborted', { statusCode: request.route.settings.response.disconnectStatusCode, data: request.response }));
 
         if (request._events) {
             request._events.emit('disconnect');

lib/server.js

@@ -349,16 +349,12 @@ internals.Server = class {
 
             res.request = custom.request;
 
-            if (custom.result !== undefined) {
-                res.result = custom.result;
-            }
-
-            if (custom.statusCode !== undefined) {
-                res.statusCode = custom.statusCode;
+            if (custom.error) {
+                throw custom.error;
             }
 
-            if (custom.statusMessage !== undefined) {
-                res.statusMessage = custom.statusMessage;
+            if (custom.result !== undefined) {
+                res.result = custom.result;
             }
         }
 

lib/transmit.js

@@ -38,9 +38,7 @@ exports.send = async function (request) {
 internals.marshal = async function (response) {
 
     for (const func of response.request._route._marshalCycle) {
-        if (response._state !== 'close') {
-            await func(response);
-        }
+        await func(response);
     }
 };
 
@@ -246,9 +244,10 @@ internals.pipe = function (request, stream) {
     const env = { stream, request, team };
 
     if (request._closed) {
-        // No more events will be fired, so we proactively close-up shop
-        request.raw.res.end(); // Ensure res is finished so internals.end() doesn't think we're responding
-        internals.end(env, 'close');
+
+        // The request has already been aborted - no need to wait or attempt to write.
+
+        internals.end(env, 'aborted');
         return team.work;
     }
 
@@ -298,14 +297,19 @@ internals.end = function (env, event, err) {
         request._core.Response.drain(stream);
     }
 
-    err = err || new Boom.Boom(`Request ${event}`, { statusCode: request.route.settings.response.disconnectStatusCode });
-    const error = internals.error(request, Boom.boomify(err));
+    // Update reported response to reflect the error condition
+
+    const origResponse = request.response;
+    const error = err ? Boom.boomify(err) :
+        new Boom.Boom(`Request ${event}`, { statusCode: request.route.settings.response.disconnectStatusCode, data: origResponse });
+
     request._setResponse(error);
 
+    // Make inject throw a disconnect error
+
     if (request.raw.res[Config.symbol]) {
-        request.raw.res[Config.symbol].statusCode = error.statusCode;
-        request.raw.res[Config.symbol].statusMessage = error.source.error;
-        request.raw.res[Config.symbol].result = error.source;       // Force injected response to error
+        request.raw.res[Config.symbol].error = event ? error :
+            new Boom.Boom(`Response error`, { statusCode: request.route.settings.response.disconnectStatusCode, data: origResponse });
     }
 
     if (event) {

test/payload.js

@@ -77,6 +77,54 @@ describe('Payload', () => {
         expect(request.response.output.statusCode).to.equal(500);
     });
 
+    it('handles aborted request mid-lifecycle step', async (flags) => {
+
+        let req = null;
+        const server = Hapi.server();
+
+        server.route({
+            method: 'GET',
+            path: '/',
+            handler: async (request) => {
+
+                req.destroy();
+
+                await request.events.once('disconnect');
+
+                return 'ok';
+            }
+        });
+
+        // Register post handler that should not be called
+
+        let post = 0;
+        server.ext('onPostHandler', () => {
+
+            ++post;
+        });
+
+        flags.onCleanup = () => server.stop();
+        await server.start();
+
+        req = Http.request({
+            hostname: 'localhost',
+            port: server.info.port,
+            method: 'get'
+        });
+
+        req.on('error', Hoek.ignore);
+        req.end();
+
+        const [request] = await server.events.once('response');
+
+        expect(request.response.isBoom).to.be.true();
+        expect(request.response.output.statusCode).to.equal(499);
+        expect(request.info.completed).to.be.above(0);
+        expect(request.info.responded).to.equal(0);
+
+        expect(post).to.equal(0);
+    });
+
     it('handles aborted request', { retry: true }, async () => {
 
         const server = Hapi.server();

test/transmit.js

@@ -561,7 +561,7 @@ describe('transmission', () => {
             const log = server.events.once('response');
             const client = Net.connect(server.info.port, () => {
 
-                client.write('GET / HTTP/1.1\r\n\r\n');
+                client.write('GET / HTTP/1.1\r\naccept-encoding: gzip\r\n\r\n');
             });
 
             const [request] = await log;
@@ -712,7 +712,7 @@ describe('transmission', () => {
 
                     this.isDone = true;
                     this.push('success');
-                    setImmediate(() => this.emit('error', new Error()));
+                    setImmediate(() => this.emit('error', new Error('stream error')));
                 };
 
                 return stream;
@@ -722,16 +722,17 @@ describe('transmission', () => {
             server.route({ method: 'GET', path: '/', handler });
             const log = server.events.once('response');
 
-            const res = await server.inject('/');
-            expect(res.statusCode).to.equal(500);
-            expect(res.statusMessage).to.equal('Internal Server Error');
-            expect(res.result.message).to.equal('An internal server error occurred');
-            expect(res.raw.res.statusCode).to.equal(200);
-            expect(res.raw.res.statusMessage).to.equal('OK');
-            expect(res.rawPayload.toString()).to.equal('success');
+            const err = await expect(server.inject('/')).to.reject(Boom.Boom);
+            expect(err.output.statusCode).to.equal(499);
+            expect(err.output.payload.error).to.equal('Unknown');
+            expect(err.output.payload.message).to.equal('Response error');
+            expect(err.data.request.response.message).to.equal('stream error');
+            expect(err.data.request.raw.res.statusCode).to.equal(200);
+            expect(err.data.request.raw.res.statusMessage).to.equal('OK');
 
             const [request] = await log;
-            expect(request.response.statusCode).to.equal(500);
+            expect(request.response.message).to.equal('stream error');
+            expect(request.response.output.statusCode).to.equal(500);
             expect(request.info.completed).to.be.above(0);
             expect(request.info.responded).to.equal(0);
         });
@@ -750,7 +751,7 @@ describe('transmission', () => {
                     this.isDone = true;
 
                     this.push('something');
-                    setImmediate(() => this.emit('error', new Error()));
+                    setImmediate(() => this.emit('error', new Error('stream error')));
                 };
 
                 return stream;
@@ -766,7 +767,8 @@ describe('transmission', () => {
 
             const [request] = await log;
             expect(err.data.res.statusCode).to.equal(200);
-            expect(request.response.statusCode).to.equal(500);
+            expect(request.response.message).to.equal('stream error');
+            expect(request.response.output.statusCode).to.equal(500);
             expect(request.info.completed).to.be.above(0);
             expect(request.info.responded).to.equal(0);
         });
@@ -1232,15 +1234,15 @@ describe('transmission', () => {
                 return h.response(stream).bytes(0);
             } });
 
-            const res = await server.inject({ url: '/stream', headers: { 'Accept-Encoding': 'gzip' } });
-            expect(res.statusCode).to.equal(499);
-            expect(res.statusMessage).to.equal('Unknown');
-            expect(res.raw.res.statusCode).to.equal(204);
-            expect(res.raw.res.statusMessage).to.equal('No Content');
-            expect(res.rawPayload.toString()).to.equal('here is the response');
+            const err = await expect(server.inject({ url: '/stream', headers: { 'Accept-Encoding': 'gzip' } })).to.reject(Boom.Boom);
+            expect(err.output.statusCode).to.equal(499);
+            expect(err.output.payload.error).to.equal('Unknown');
+            expect(err.output.payload.message).to.equal('Request close');
+            expect(err.data.request.raw.res.statusCode).to.equal(204);
+            expect(err.data.request.raw.res.statusMessage).to.equal('No Content');
 
             const [request] = await log;
-            expect(request.response.statusCode).to.equal(499);
+            expect(request.response.output.statusCode).to.equal(499);
             expect(request.info.completed).to.be.above(0);
             expect(request.info.responded).to.equal(0);
 
@@ -2056,7 +2058,7 @@ describe('transmission', () => {
 
     describe('chain()', () => {
 
-        it('handles stream errors on the response after the response has been piped (http)', async () => {
+        it('handles stream errors on the response after the response has been piped', async () => {
 
             const handler = (request, h) => {
 
@@ -2079,8 +2081,8 @@ describe('transmission', () => {
             const server = Hapi.server({ compression: { minBytes: 1 } });
             server.route({ method: 'GET', path: '/', handler });
 
-            const res = await server.inject({ url: '/', headers: { 'accept-encoding': 'gzip' } });
-            expect(res.statusCode).to.equal(500);
+            const err = await expect(server.inject({ url: '/', headers: { 'accept-encoding': 'gzip' } })).to.reject(Boom.Boom);
+            expect(err.output.statusCode).to.equal(499);
         });
     });
 });