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: ignore multiple abort #28706

Closed
wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Jul 15, 2019

All calls expect the first to abort should be a noop. abort event should not be emitted more than once.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jul 15, 2019
@@ -31,6 +31,8 @@ server.listen(0, common.mustCall(() => {
const options = { port: server.address().port };
const req = http.get(options, common.mustCall((res) => {
res.on('data', (data) => {
req.on('abort', common.mustCall());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this actually test the change? As far as I can tell 'abort' was emitted only once even before. Can we have a better test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? I call abort() twice and 'abort' is only emitted once...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's my point. it works like this even without the changes proposed here, so it is not clear what this is testing.

Copy link
Member

@lpinca lpinca Jul 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'use strict';

const http = require('http');

const server = http.createServer(function(req, res) {
  res.flushHeaders();
});

server.listen(function() {
  const req = http.get({ port: server.address().port });

  req.on('response', function(res) {
    const dump = res._dump;

    res._dump = function() {
      console.log('dump');
      dump.apply(res, arguments);
    };

    req.abort();
    req.abort();
    req.abort();
    req.abort();
  });

  req.on('abort', function() {
    console.log('abort');
  });
});

'abort' is already emitted only once. The difference is that res._dump() is only called once now. Hope it makes sense.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Updated. Not pretty but I can't see any other way.

@ronag ronag force-pushed the http-fix-abort-once branch 5 times, most recently from 0c9fb90 to 228b7a2 Compare July 17, 2019 19:20
@ronag ronag force-pushed the http-fix-abort-once branch 2 times, most recently from c41bb9f to ea27fc4 Compare July 17, 2019 20:59
@nodejs-github-bot
Copy link
Collaborator

res.on('data', (data) => {
req.abort();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to make this a separate test rather than modifying the existing test? My concern is that the existing test is such a straightforward one and this kind of makes it less obvious what's going on. (There's no comment explaining why req.abort() is being called twice. But also wouldn't we want a test where it is only called once?)

@trivikr trivikr requested a review from mscdex July 17, 2019 22:46
@ronag ronag mentioned this pull request Aug 6, 2019
9 tasks
@ronag
Copy link
Member Author

ronag commented Aug 24, 2019

This is sorted better by #29192

@ronag ronag closed this Aug 24, 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

Successfully merging this pull request may close these issues.

7 participants