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

pause() on response of http.request() causes 'aborted' event, and ClientRequest.abort() resumes the response. #10961

Closed
duanyao opened this issue Jan 23, 2017 · 7 comments
Labels
http Issues or PRs related to the http subsystem.

Comments

@duanyao
Copy link
Contributor

duanyao commented Jan 23, 2017

  • Version: v7.4.0
  • Platform: Windows 10 64bit
  • Subsystem: http

Test case:

var request = require('http').request;
var mUrl = require('url');
var url = 'http://example.com'; // or any url

var options = {
  host: mUrl.parse(url).hostname,
  method: 'GET',
  path: url,
};

//console.log('options:', options);

var req = request(options, res => {
  console.log('got response, url=' + url + ', code=' + res.statusCode);
  res.pause();
  res.on('data', (chunk) => {
    console.log('on data:' + chunk.length);
  });
  res.on('end', () => {
    console.log('end');
  });
  res.on('aborted', err => {
    console.log('on aborted');
    setTimeout(()=> console.log('...'), 10000);

    console.log('res.isPaused() before req.abort():' + res.isPaused());
    req.abort();
    console.log('res.isPaused() after req.abort():' + res.isPaused());
  });
});

req.on('error', err => {
  console.error('on error:', err);
});

req.end();

Actual result (stdout):

got response, url=http://example.com, code=200
on aborted
res.isPaused() before req.abort():true
res.isPaused() after req.abort():false
on data:1097
on data:173
end

Note that "on aborted" happens almost immediately after res.pause().

Expected result:

No "aborted" event is fired if the response is not acutally aborted, and ClientRequest.abort() shouldn't resume the response.

I use Wireshark to monitor the traffic, and there is nothing wrong with the response, even if I don't call req.abort().

@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Jan 23, 2017
@bnoordhuis
Copy link
Member

You get that 'aborted' event because the server closes the connection while paused on your end, not because you call req.abort(). If that is not what you are seeing locally, please provide the exact steps for me to reproduce.

No "aborted" event is fired if the response is not acutally aborted, and ClientRequest.abort() shouldn't resume the response.

Can you explain why you expect that? Why shouldn't req.abort() unpause the request? It's dead anyway.

@duanyao
Copy link
Contributor Author

duanyao commented Jan 25, 2017

You get that 'aborted' event because the server closes the connection while paused on your end, not because you call req.abort()

No, the server didn't close the connection prematurely. As I said above, Wireshark showed that the complete response was received without problem, regardless the 'aborted' event.

Do you think 'aborted' event should be fired in this case? I don't think so.

  • server is sending the response;
  • client calls response.pause();
  • the response is sent completely, server closes connection;
  • 'aborted' event on the response;

If that is not what you are seeing locally, please provide the exact steps for me to reproduce.

Just run the test code above and you'll see the problem always.

Why shouldn't req.abort() unpause the request? It's dead anyway.

Yeah, the request should have died (but seemed not according to Wireshark), but req.abort() brought it to life again ('data' events after 'aborted' event), that's the problem.

@bnoordhuis
Copy link
Member

No, the server didn't close the connection prematurely.

I didn't use the word 'prematurely.'

Do you think 'aborted' event should be fired in this case? I don't think so.

No strong opinion. If you want to see it changed, you should present a compelling reason why. Keep in mind there is probably code out there that depends on the current behavior.

@duanyao
Copy link
Contributor Author

duanyao commented Jan 25, 2017

'Abort', by defination, is premature. No? If you don't think so, please explain what is aborted in this case, exactly?

The problem is, l can't tell the difference between premature and normal closing of the connection if pause() is called.

lf this behavior can't be changed, l'd ask to add a new event to act as what 'aborted' should do.

@duanyao
Copy link
Contributor Author

duanyao commented Jan 25, 2017

P.S., l am still not sure about the exact behavior of 'aborted' event. No matter it can be changed or not, it need to be documented in detail.

@Trott
Copy link
Member

Trott commented Jul 16, 2017

@nodejs/http

@apapirovski
Copy link
Member

This was fixed in 1980a36#diff-e3bc37430eb078ccbafe3aa3b570c91a

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

5 participants