Debugging: Stabilize hidden classes for improved heap snapshots

[Raynos]

• Version: 4.4.7
• Platform: linux
• Subsystem: http

Currently the http client does not have stable hidden classes so it's hard to debug it.

For example:

• https://github.com/nodejs/node/blob/master/lib/_http_client.js#L233
• https://github.com/nodejs/node/blob/master/lib/_http_client.js#L229

These are two different fields that do not exist in the constructor ( https://github.com/nodejs/node/blob/master/lib/_http_client.js#L18-L202 ).

We need to add self.res = null; self.aborted = 0;

Without a stable hidden class for the ClientRequest ( one that doesn't change at runtime ) it's very hard to debug the state of a heap using a heapsnapshot

This heapsnapshot is missing the res and aborted fields. Having these fields available would make it a lot easier to reason about the state of the ClientRequest ( I have a memory leak where I am leaking outbound ClientRequest instances ).

I'm sure that this problem exists for a lot of other internal classes within the code.

[MylesBorins]

@Raynos is this problem in v6 as well? Would the solution be simply adding those fields in the constructor?

[Raynos]

This applies to 4.x & master. I did not check 6.x, I assume 6.x and master are the same.

[MylesBorins]

/cc @nodejs/http

[cjihrig]

Adding the good first contribution label.

[shmuga]

Can take this one.
But what should we do with static variable listed in _http_client?
I tried to remove it locally and run tests and they passed. But is that right decision?

[Raynos]

Personally, having variables defined on the prototype is more messy then defining them in the constructor.

[AnnaMag]

@shmuga are you working on this?

[benjamingr]

It's also slower to define them on the prototype. It's a good idea to always define properties used statically - even as null.

[Fishrock123]

Doing this would probably also help performance.