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

"disconnect" event on the "request" object is never emitted #3528

Closed
jfromaniello opened this issue Jun 19, 2017 · 3 comments
Closed

"disconnect" event on the "request" object is never emitted #3528

jfromaniello opened this issue Jun 19, 2017 · 3 comments
Assignees
Labels
Milestone

Comments

@jfromaniello
Copy link

@jfromaniello jfromaniello commented Jun 19, 2017

This is a simple test where I create a HAPI server with a route that replies with "hello world" after 2 seconds. The same code contains a client that does a request to the route and aborts it after 500ms.

const request = require('request');
const Hapi = require('hapi');

const server = new Hapi.Server();

server.connection({
  host: 'localhost',
  port: 3001,
});

server.route({
  method: 'GET',
  path:'/slow',
  handler: function (request, reply) {
    request.once('disconnect', () => {
      console.log('disconnected');
    });
    setTimeout(() => reply('hello world'), 2000);
  }
});

server.start();

setTimeout(() => {
  console.log('starting request');
  const req = request.get('http://localhost:3001/slow');
  setTimeout(() => {
    console.log('aborting request');
    req.abort();
  }, 500);
}, 1000);

According to the documentation the request object should emit a disconnect event. After debugging for a while I notice that HAPI subscribes to the event too late when the route calls the reply function.

I have tried with version hapi@16.4.3 and node v6.

Thank you very much in advance.

@jfromaniello jfromaniello changed the title "disconnect" event on the "request" object is never fired "disconnect" event on the "request" object is never emitted Jun 19, 2017
@AJamesPhillips
Copy link
Contributor

@AJamesPhillips AJamesPhillips commented Jun 22, 2017

I tried to recreate this and succeeded. I also tried using the python requests with requests.get("http://localhost:3001/slow", timeout=1) and using net as per the hapi tests.

What are you trying to achieve or the steps to reproduce?

// https://github.com/hapijs/hapi/issues/3528

// const request_module = require('request');
const Hapi = require('hapi');
const Net = require('net');

const server = new Hapi.Server();

server.connection({
  host: 'localhost',
  port: 3001,
});

const onRequest = function (request, reply) {

    request.once('disconnect', () => {

        console.log('server: request disconnected');
    });

    request.once('finish', () => {

        console.log("server: request finished");
    });

    return reply.continue();
};

server.ext('onRequest', onRequest);

server.route({
    method: 'GET',
    path: '/slow',
    handler: function (request, reply) {

        console.log("server: handling request");

        setTimeout(() => {

            console.log("server: replying");
            reply('hello world');
        }, 200);
    }
});

server.start((err) => {

    if (err) {
        console.error('server: error', err);
        process.exit(1);
    }

    console.log('server: started');

    const client = Net.connect(3001, 'localhost', () => {

        console.log('client: starting request');
        client.write('GET /slow HTTP/1.1\r\n\r\n');
        console.log('client: starting request');
        client.write('GET /slow HTTP/1.1\r\n\r\n');
    });

    client.on('data', (data) => {

        console.log('client: received data:', data);
    });

    setTimeout(() => {

        console.log('client: destroying client');
        client.destroy();
    }, 100);
});

What was the result you received?

server: started
client: starting request
client: starting request
server: handling request
server: handling request
client: destroying client
server: replying  # <=
server: replying  # <=

What did you expect?

server: started
client: starting request
client: starting request
server: handling request
server: handling request
client: destroying client
server: request disconnected  # <=
server: request disconnected  # <=

@devinivy
Copy link
Member

@devinivy devinivy commented Jun 26, 2017

This does appear to be a bug to me. The request's close/abort listeners aren't added until transmission:

hapi/lib/transmit.js

Lines 269 to 290 in c212bc8

if (event || err) {
request.emit('disconnect');
}
request._log(tags, err);
return callback();
};
source.once('error', end);
const onAborted = () => {
return end(null, 'aborted');
};
const onClose = () => {
return end(null, 'close');
};
request.raw.req.once('aborted', onAborted);
request.raw.req.once('close', onClose);

The current hapi test doesn't cause the disconnect until transmission has begun:

hapi/test/request.js

Lines 334 to 338 in b8efbd9

client.on('data', () => {
--total;
client.destroy();
});

It seems that the close/abort listeners that trigger the disconnect event should be moved roughly here:

hapi/lib/request.js

Lines 171 to 178 in b8efbd9

this._onClose = () => {
this._log(['request', 'closed', 'error']);
this._isPayloadPending = false;
this._isBailed = true;
};
this.raw.req.once('close', this._onClose);

@devinivy devinivy added the bug label Jun 26, 2017
@hueniverse hueniverse self-assigned this Aug 3, 2017
@hueniverse hueniverse added this to the 16.5.1 milestone Aug 3, 2017
@hueniverse hueniverse closed this in a9fb7e0 Aug 3, 2017
@jfromaniello
Copy link
Author

@jfromaniello jfromaniello commented Aug 3, 2017

Thanks a lot!

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants