Cleanup: check activity of Http2Stream by ActivityCop by masaori335 · Pull Request #6494 · apache/trafficserver

masaori335 · 2020-03-10T00:40:54Z

Introduce NetTimeout and ActivityCop to cleanup Http2Stream.

For now, Http2Stream is checking the activity and inactivity by itself. This is working but giving little pressure to the EventSystem and complexity of code.
In the long term, HTTP/3 & QUIC need a similar timeout mechanism (probably, QUICStreamVConnection). ActivityCop is made as reusable easily.

active timeout of Http2Stream

trafficserver/proxy/http2/Http2Stream.cc

Line 828 in deea85a

active_event = this_ethread()->schedule_in(this, active_timeout);
inactive timeout of Http2Stream

trafficserver/proxy/http2/Http2Stream.cc

Line 839 in deea85a

inactive_event = this_ethread()->schedule_every(this, HRTIME_SECONDS(1));

Out of scope

NetTimeout/ActivityCop is very similar to NetEvent/InactiviyCop which observes the activity and inactivity of UnixNetVConnection. We can make it NetTimeout/ActivityCop base, but it needs a bunch of refactoring of the iocore layer. I leave it for now.

maskit

The interface doesn't look great to me.

maskit · 2020-03-10T08:19:49Z

+    }
+
+    if (t->is_inactive_timeout_expired(now)) {
+      t->handleEvent(VC_EVENT_INACTIVITY_TIMEOUT, e);


What information in e is needed?

maskit · 2020-03-10T09:17:49Z

-    active_event->cancel();
-    active_event = nullptr;
-  }
+  return _timeout.is_active_timeout_expired(now);


What benefit do we get from delegating this?

If Http2Stream inherited NetTimeout, we would not need to use template and the check that requires C++20. And the interface of NetTimeout would be clearer. Currently it has functions that are indirectly called from ActivityCop and functions that are called from Http2Stream to update internal state. These two kinds of function would be separated as public functions for ActivityCop and protected functions for Http2Stream. Then ActivityCop would be able to call is_active_timeout_expired directly and we would not need this function here.

To make ActivityCop without Template, you need a class that inherits VConnection & NetTimeout. When Http2Stream inherits that class, you'll see the diamond problem. Also, when you try to make ActivityCop based InactivityCop, your class hierarchy becomes more complicated.

You can solve the complicated class hierarchy. But using Template is easier than that.

Which part in ActivityCop requires VConnection interface?

If you are talking about VC_EVENTs, it's not checked on your code neither. And technically the class doesn't have to be a VConnection. Continuation satisfies the requirement for handling events.

If that part is going to be a problem, you could add on_timeout() to the interface. It's more general and you would be able to use it for any timeout.

Yeh, it's Continuation.
Assuming you're saying NetTimeout doesn't need to inherit the Continuation by declaring int on_timeout(int event, Event *e) as a pure virtual function. It will work, but it allows derived class to implement it anyhow. Some will call Continuation::handleEvent(int, void*) correctly, but some will not.
I prefer to restrict how events are passed & handled.

Calling handleEvent would not ensure that timeout is going to be handled correctly. You still need to implement the handler correctly. In this sense, having handleEvent is not enough, and I'd call it a halfway restriction. on_timeout() would be more robust and flexible interface because it does not depend on other mechanisms. Changes against how we handle timeout on VConnection does not affect ActivityCop's job, and like I said on the comment above, you can use AC for other timeouts.

Also, having on_timeout and having handleEvent at somewhere are not exclusive. You could implement on_timeout to (Net)VConnection if you wanted to make sure handleEvent is going to be called at least.

maskit · 2020-03-10T09:32:34Z

+}
+
+inline bool
+NetTimeout::is_inactive_timeout_expired(ink_hrtime now)


Why is this check done by NetTimeout itself?

The interaction between ActivityCop and NetTimeout looks weird.
AC: Hey, it's March 1st. Is your visa expired?
NT: Yes, it is.
AC: Your visa is expired.
NT: (I know, I told you.)

I'd do
AC: Hey, when is your visa expire date?
NT: It's March 1st.
AC: Your visa is expired.

The main motivation is hiding private members as much as possible. Maybe I did too much.

I revisit this but it looks like what AC needs to know is timeout is expired or not.
We see a similar code & relations for checking a connection is closed or not.

Just asking expiration time is enough to know whether a stream is timed out. I don't see any needs to make two function calls if NetTimeout already know it's timed out. I'd move the check logic to AC and make NetTimeout to a simple data structure (with accessors).

shinrich

I agree that moving the interval-based activity checking to the session level from the stream level makes sense.

masaori335 · 2020-03-11T02:58:33Z

Pushed new commit. Including headers & some function of NetTimeout was tweaked a bit.

maskit · 2020-03-11T06:06:40Z

I think it could be better, but I don't see blockers and it's better than current code. I'm not going to stop it but no +1 from me at the moment.

scw00

Looks good to me. After this change , Timeout Event : H2Stream = 1 : n. I'd like to launch this change , and change it again after we fully abstract the Timer interface.

masaori335 added HTTP/2 Network labels Mar 10, 2020

masaori335 added this to the 10.0.0 milestone Mar 10, 2020

masaori335 self-assigned this Mar 10, 2020

masaori335 force-pushed the activity-cop-2 branch from bb9f4e3 to 3ac0f62 Compare March 10, 2020 01:47

maskit reviewed Mar 10, 2020

View reviewed changes

shinrich previously approved these changes Mar 10, 2020

View reviewed changes

Cleanup: check activity of Http2Stream by ActivityCop

6e20007

masaori335 dismissed shinrich’s stale review via 6e20007 March 11, 2020 02:35

masaori335 force-pushed the activity-cop-2 branch from 3ac0f62 to 6e20007 Compare March 11, 2020 02:35

scw00 approved these changes Mar 12, 2020

View reviewed changes

shinrich approved these changes Mar 12, 2020

View reviewed changes

masaori335 merged commit c05e751 into apache:master Mar 13, 2020

maskit mentioned this pull request Aug 13, 2020

Assertion failure: event == HTTP_TUNNEL_EVENT_DONE in HttpSM #7102

Closed

masaori335 mentioned this pull request Aug 13, 2020

Cancel active/inactive timeout on closing Http2Stream #7111

Merged

zwoop modified the milestones: 10.0.0, 9.1.0 Jan 28, 2021

Conversation

masaori335 commented Mar 10, 2020

Out of scope

Uh oh!

maskit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maskit Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shinrich left a comment

Choose a reason for hiding this comment

Uh oh!

masaori335 commented Mar 11, 2020

Uh oh!

maskit commented Mar 11, 2020

Uh oh!

scw00 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

maskit Mar 11, 2020 •

edited

Loading