http: response does not emit error on premature close

[ronag]

Consider the following example:

const server = http.createServer((req, res) => {
  setInterval(() => {
    res.write(Buffer.alloc(64))
  }, 1000)
  setTimeout(() => res.destroy(), 10000)
});

server.listen(0, '127.0.0.1', async () => {
  let dst = fs.createWriteStream('/tmp/test')
  try {
	  await new Promise((resolve, reject) => http
	    .get({
	      host: '127.0.0.1',
	      port: this.address().port
	    }, res => {
	      res
	        .on('error', reject)
	        .pipe(dst)
	        .on('error', reject)
	        .on('finish', resolve)
	    })
	    .on('error', reject)
	  )
  } finally {
    dst.destroy()
  }
})

Unintuitively, this will just hang and never finish (post #27984) or unexpectedly not fail and create an incomplete file (pre #27984). This is because we are not subscribing to res.'aborted'. If we have not yet received response we will get req.'error' with ECONNRESET and if we have received a response we only get an res.'aborted' and no 'error'.

In the above example (which is how I believe most people assume it should be) we have a problem. This has been a common source of error both for myself and other developers I have worked with (although in 99% of the time it's not a problem or not noticed).

Would it make sense to deprecate aborted and make the response object behave the same way as the request object, i.e. emit an error on premature close? Which I believe would be more consistent with the streams API.

Alternatively, maybe a warning/note in the documentation or some other means of avoiding people making this mistake?

Currently the proper way to implement this is either:

  let dst = fs.createWriteStream('/tmp/test')
  try {
	  await new Promise((resolve, reject) => http
	    .get({
	      host: '127.0.0.1',
	      port: this.address().port
	    }, res => {
	      res
	        .on('error', reject)
	        .on('aborted', () => reject(new Error('premature close')))
	        .pipe(dst)
	        .on('error', reject)
	        .on('finish', resolve)
	    })
	    .on('error', reject)
	  )
  } finally {
    dst.destroy()
  }  

Or post #27984:

await new Promise((resolve, reject) => http
  .get({
    host: '127.0.0.1',
    port: this.address().port
  }, res => {
    stream.pipeline(
     res, 
     fs.createWriteStream('/tmp/test'), 
     err => err ? reject(err) : resolve()
    )
  })
  .on('error', reject)
)

[ronag]

ping @Trott. Any feedback on this?

[Trott]

/ping @nodejs/http, in particular on this:

In the above example (which is how I believe most people assume it should be) we have a problem. This has been a common source of error both for myself and other developers I have worked with (although in 99% of the time it's not a problem or not noticed).

Would it make sense to deprecate aborted and make the response object behave the same way as the request object, i.e. emit an error on premature close? Which I believe would be more consistent with the streams API.

Alternatively, maybe a warning/note in the documentation or some other means of avoiding people making this mistake?

[Trott]

Warning note in the doc would probably be relatively uncontroversial. If a change in behavior is deemed appropriate, we would probably need to wait for 13.0.0 anyway (and the warning can be removed at that time).

[ronag]

@Trott How do I bring this issue forward? Or if it is being followed up where can I see that? Is there an agenda or something for node/http?

If it's decided that we are not doing anything further that is also fine. I'd just like to know.

[Trott]

@Trott How do I bring this issue forward? Or if it is being followed up where can I see that? Is there an agenda or something for node/http?

Unfortunately @nodejs/http is one of the less active teams. (And one of the people on it that is, in fact, rather active in Node.js core happens to be taking some time off right now.) So it may be necessary to ping a bit more widely to get some activity on this. There's an unspoken rule that, in most cases, you don't ping @nodejs/collaborators without at least first pinging a more focused group. But since we've pinged that more focused group and haven't gotten much movement...

@nodejs/collaborators This could use some comments, or even just emoji reactions to be honest.

[ronag]

Unfortunately @nodejs/http is one of the less active teams.

That’s unfortunate. As you might have noticed. I have quite a lot of things I would like to help sorting out in this area.

[benjamingr]

@ronag feel free to ping me directly and I will do my best to try and review.

[mcollina]

I'm all in for making things less custom. However I'm a bit worried about the potential breakages caused by emitting a custom error in place of a 'aborted'. One change that we could introduce is to keep the existing behavior if a listener to 'aborted' is present, and emit 'error' if there is no listener to 'aborted'.

Wdyt?

[ronag]

I'm all in for making things less custom. However I'm a bit worried about the potential breakages caused by emitting a custom error in place of a 'aborted'. One change that we could introduce is to keep the existing behavior if a listener to 'aborted' is present, and emit 'error' if there is no listener to 'aborted'.

Wdyt?

I love this.

[leodutra]

It just happened to me, and it took me like 6 hours to find the issue.
As I was using stream.pipeline, I thought I had my back covered and started looking for the error on hypothetically less consistent libs, like the one I use to show progress on the CLI.

But stream.pipeline won't handle the 'abort' event.
And this is tricky, because when using third-party libraries, sometimes we may not know/remember the response stream we're using is actually a http.ClientRequest, which extends a Writable stream and that adds some events, like the 'abort'... which is not treated as an 'error'.

Emitting 'error' for 'abort' makes semantic sense to me, as 'timeout' also emits 'abort'.
A timeout (or an abort) is not the exactly expected answer from a request.
And in the end, streaming events this way would cover the back of any unadvised dev.

[ronag]

@leodutra: Do you have an example and which version of node are you using? This should not be a problem if you are using pipeline and Node 13+.

Though I would still very much resolve this issue for the non pipeline case.

[leodutra]

I created a personal node-utils repo (which still don't have tests, but are being used every day on chatbots and some other PoCs).

There is a node-utils/downloadFile.js

When downloading files with GB, I noticed streams just stopped updating the progress bars and there started my investigation. Yesterday I tested a local version with abort, and it at least showed me the server dropped my connection... so the error is now being handled.

This happened at the v12.12.0 and now on v13.0.1 for sure.

[leodutra]

FYI, I removed the cli-progress and got, on some local testing, and still could see 'data' stop flowing without any 'error'.

[ronag]

@leodutra: This seems to be something in got which is not entirely correct. Maybe related to this issue, unsure. There is a bit too much going on inside of got for me to figure out from a quick glance.

[ronag]

@leodutra: This seems to be something in got which is not entirely correct. Maybe related to this issue, unsure. There is a bit too much going on inside of got for me to figure out from a quick glance.

I would avoid got and try something like https://gist.github.com/ronag/276513d01dbf5e551d73e89ff7f79dac

If you want further help feel free to reach out to me on Slack.

[leodutra]

Yeah, I'm actually building a small request to stream with just the parts I need.
I started using got just to make the process faster, skipping the handling of http/https and redirects.

I'll keep digging into it, I also need to clean the CLI bars for promise errors, in that particular source.

Thanks for the source, @ronag.

[leodutra]

I did test a bit more and looks like I was wrong. When using stream.pipeline(), at least on Node v13.0.1, the 'aborted' will trigger the error handling.

I was able to see the error when I fixed the control of the CLI progress (the redraw was swallowing other console messages).

But without pipeline, 'aborted' trouble comes again.

Thanks for the help, @ronag.

[imcotton]

@ronag

Hey, btw you could grab out the response from request with events.once:

const { get } = require('http');
const { once } = require('events');

const req = get({ host: '127.0.0.1', port: 8080 });

const [ res ] = await once(req, 'response');

Which, handles error as rejection internally.