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

http: missing close after finish #27480

Closed
ronag opened this issue Apr 29, 2019 · 8 comments
Closed

http: missing close after finish #27480

ronag opened this issue Apr 29, 2019 · 8 comments
Labels
http Issues or PRs related to the http subsystem.

Comments

@ronag
Copy link
Member

ronag commented Apr 29, 2019

Node 10.13

const http = require('http')
const send = require('send')

const server = http
  .createServer(async (req, res) => {
    send(req, '/Users/ronagy/Desktop/old/AFT2/Start_060918_40.mov')
      .on('error', err => console.error(err))
      .pipe(res)
      .on('error', err => console.error(err))
      .on('finish', () => console.log('finish'))
      .on('close', () => console.log('close'))
  })

server.listen(9000, () => {
  http.request({
    method: 'GET',
    hostname: '0.0.0.0',
    port: 9000
  }).on('response', res => {
    res.on('data', buf => {
      // drain
    })
  }).end()
})

Would expect it to print 'finish' and then 'close'.

@ronag ronag changed the title missing close after finish http: missing close after finish Apr 29, 2019
@ronag
Copy link
Member Author

ronag commented Apr 29, 2019

A bit more nodejs testy:

const http = require('http')
const send = require('send')

const server = http
  .createServer(common.mustCall((req, res) => {
    send(req, '/Users/ronagy/Desktop/old/AFT2/Start_060918_40.mov')
      .pipe(res)
      .on('finish', common.mustCall())
      .on('close', common.mustCall());
  }));

server.listen(9000, common.mustCall(() => {
  http.get({ port: server.address().port }, res => {
    res.on('data', buf => {
      // drain
    });
  });
}));

@lpinca
Copy link
Member

lpinca commented Apr 30, 2019

I can't reproduce:

$ dd if=/dev/urandom of=data.bin count=1024 bs=1024
1024+0 records in
1024+0 records out
1048576 bytes transferred in 0.030293 secs (34614479 bytes/sec)
$ cat finish.js 
'use strict';

const http = require('http');
const { createReadStream } = require('fs');

const server = http.createServer((req, res) => {
  createReadStream('./data.bin')
    .pipe(res)
    .on('finish', () => {
      console.log('finish');
    })
    .on('close', () => {
      console.log('close');
    });
});

server.listen(0, () => {
  http.get({ port: server.address().port }, (res) => {
    res.resume();
  });
});
$ node finish.js 
finish
close
^C

@lpinca lpinca added the http Issues or PRs related to the http subsystem. label Apr 30, 2019
@ronag
Copy link
Member Author

ronag commented Apr 30, 2019

dd if=/dev/urandom of=data.bin count=1024 bs=1024
1024+0 records in
1024+0 records out
1048576 bytes transferred in 0.029004 secs (36152686 bytes/sec)
const http = require('http')
const { createReadStream } = require('fs')

const server = http
  .createServer((req, res) => {
    createReadStream('./data.bin')
      .pipe(res)
      .on('finish', () => console.log('finish'))
      .on('close', () => console.log('close'))
  })

server.listen(9000, () => {
  http.get({ port: server.address().port }, res => {
    res.resume()
  })
})
storage$ node --version
v10.13.0
storage$ node tmp.js 
finish
^C

@ronag
Copy link
Member Author

ronag commented Apr 30, 2019

storage$ node --version && node tmp.js 
v10.15.3
finish
^C

@ronag
Copy link
Member Author

ronag commented Apr 30, 2019

Works in node 11 and later. Not sure what commit fixed it?

storage$ node --version && node tmp.js 
v12.1.0
finish
close
node --version && node tmp.js 
v11.14.0
finish
close

@ronag
Copy link
Member Author

ronag commented Apr 30, 2019

@mcollina is this something we want to fix in LTS or should I just drop it?

@lpinca
Copy link
Member

lpinca commented Apr 30, 2019

@ronag I think it is f22c7c1 (#20611) but it was reverted from v10.

@ronag
Copy link
Member Author

ronag commented Apr 30, 2019

aah! yes, then it is as it should be

@ronag ronag closed this as completed Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

No branches or pull requests

2 participants