tls: follow up for #5168

[indutny]

This is a promised follow-up for #5168 PR.

cc @nodejs/crypto

R = @bnoordhuis

[indutny]

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

[bnoordhuis]

src/stream_base.h:114: The class StreamResource probably needs a virtual destructor due to having virtual method(s), one declared at line 151. [runtime/virtual] [4]

EDIT: 6807dc1 fixes that.

[bnoordhuis]

Adding nullptr checks everywhere is often an indication that one needs to rethink their object life cycle management... IOW, is this really the best possible fix?

[indutny]

@bnoordhuis yeah, there are conceptual problems that I haven't really figured out a best solution for yet. Perhaps, we could try to brainstorm it here together?

So basically the current scheme involves two objects: TLSWrap and TCPWrap that depend on each other. The bad thing is that they have different lifetime: TCPWrap::Close (inherited from HandleWrap) destroys the TCPWrap instance asynchronously, and TLSWrap is left with a dangling pointer to TCPWrap.

Why is cross-reference even needed? This is the way StreamBase works: there are callbacks and methods on TCPWrap instance. So in order to parse and override its input, one need to be able to listen for all data events (with OnRead and OnAlloc), and re-emit proper data events too (with tcpwrap->EmitData).

So the solution should either be re-thinking the StreamBase, making their lifetimes match, or adding close callback as in this PR.

I have no ideas for StreamBase re-thinking, it seems that matching lifetimes will very likely kill the reason why TCPWrap::Close deallocates TCPWrap (decrease memory consumption early), so I just did it as in this PR.

@bnoordhuis do you have any ideas?

[jasnell]

@bnoordhuis ... ping

[bnoordhuis]

No concrete suggestions, but I suspect a more rigorous refactoring is in order. This PR is really just a band-aid; it might be a quick fix but piling on workarounds doesn't result in maintainable software.

[jasnell]

ping @indutny ... do you still want to pursue this?

[fhinkel]

Feel free to reopen this PR if you get back to working on it.