http2: add Http2Session.lastReceivedTime

[jasnell]

Adds a time stamp indicating the last time data was received from
the connected peer.

Fixes: #21730

/cc @murgatroid99 who requested the feature
/cc @nodejs/http2

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]

@jasnell build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/968/pipeline

[jasnell]

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

[mcollina]

LGTM

[addaleax]

Hm … given that this isn’t specific to HTTP/2 as a protocol, would it make sense to implement this for net.Sockets in general, and forward it on the HTTP/2 session?

[jasnell]

Hm … given that this isn’t specific to HTTP/2 as a protocol, would it make sense to implement this for net.Sockets in general, and forward it on the HTTP/2 session?

maybe? lol ... perhaps this for now and if we receive any requests to generalize it we can fairly easily.

[BridgeAR]

The addition itself seems like a good thing but I would prefer to have it as a function call named getLastReceivedTime(). It's already implemented as getter and making it a function reduces the chance that a user has a typo when using this.

I also like the suggestion from @addaleax to make it a general thing for net.Socket.

[jasnell]

Making it a generalized thing for net.Socket can be done later in a separate PR if there is a demand for it.

[addaleax]

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/239/

[jasnell]

@mcollina and @ryzokuken ... changed to a function to address @BridgeAR's feedback. PTAL

[jasnell]

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

[addaleax]

Benchmark CI shows a few small but significant drops, here’s one with more iterations: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/240/

[jasnell]

Yeah, I was afraid of that on the perf drop. Before this lands I'll investigate how I can minimize that

[lundibundi]

@jasnell should author ready be removed then to avoid confusion and someone landing this before you finish?

[jasnell]

Yes. Let's do that On Sep 24, 2018 07:01, "Denys Otrishko" <[email protected]> wrote: @jasnell <https://github.com/jasnell> should author ready be removed then to avoid confusion and someone landing this before you finish? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23010 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAa2ea43YQLedyrjwcOCnZxD73rheO06ks5ueOXNgaJpZM4W09v8> .

[ryzokuken]

LGTM after the update suggested by @BridgeAR

[jasnell]

Going to close this and revisit it later.