http: subclass of Agent can override the initialization

[fengmk2]

Make userland's Agent inherit from http.Agent more easily.

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

Affected core subsystem(s)

[fengmk2]

Just move "free" listener into "init()" method.
And replace var to let and const.

[TimothyGu]

The changes to use this instead of self, let/const instead of var, etc. don't look related to making Agent subclassable, nor are they justified from a performance standpoint. Besides the unrelated changes, the factoring-out of init() also looks ad-hoc to the specific usage of DestroyAgent, though I'll defer to the judgement of @nodejs/http on this.

[fengmk2]

@TimothyGu revert all replacement.

[fengmk2]

https://github.com/request/tunnel-agent/blob/master/index.js#L47 Now TunnelingAgent can inherits from http.Agent more easily.

function TunnelingAgent extends http.Agent {
  constructor(options = {}) {
    super(options);

    this.proxyOptions = this.options.proxy || {}
    this.maxSockets = this.options.maxSockets || http.Agent.defaultMaxSockets
  }

  init() {
    this.on('free', function onFree(socket, host, port) {
      // override the free socket listen
    });
   }
  
  // others methods
  addRequest() {}
}

oudated review

[joyeecheung]

ping @nodejs/http

[lpinca]

@fengmk2 is this superseded by #11567?

[joyeecheung]

@lpinca I don't think so, #11567 relaxed the constraint of extended agents, this PR makes the initialization part of the agents customizable via the init() method. Although the ongoing #13005 also tries to make HTTP agents more overridable as well.

This needs a rebase before anyone familiar with this part of the codebase can give a review...@fengmk2

[fengmk2]

@joyeecheung :)

I fixed the conflicts and push again. Anyone can review now!

[BridgeAR]

PTAL @nodejs/collaborators @nodejs/http

[jasnell]

oy! completely missed this one, sorry @fengmk2

[jasnell]

This likely needs a doc update. Code looks fine on first read through, needs a CI

[jasnell]

CI: https://ci.nodejs.org/job/node-test-pull-request/9861/

[fengmk2]

@jasnell The ci is still running for a long time.

[jasnell]

the ci completed, the results just are not posting appropriately back to Github.
Here's a fresh run: https://ci.nodejs.org/job/node-test-pull-request/9966/

[mcollina]

Can you please explain why you need sidestep the internal 'free' event handler? That part is critical for the logic of http.Agent. What is your use case? Maybe we are missing another event or method.

I am -1 with this change.

[BridgeAR]

Ping @fengmk2

[BridgeAR]

@fengmk2 do you still want to pursue this?

[BridgeAR]

I am closing this due to long inactivity. @fengmk2 if you would like to pursue this, please reopen.