quiche:: Implement QuicAlarm interface #7094

danzh2010 · 2019-05-28T23:23:02Z

Implement EnvoyQuicAlarm and EnvoyQuicAlarmFactory. They are backed by Event::Scheduler.

Added a new interface enableTimerInUs() in Timer to allow smaller granularity.

Risk Level: middle, Timer::enableTimer() behavior is changed.
Testing: Added tests under test/extensions/quic_listeners/quiche:envoy_quic_alarm_test; enableTimerInUs() covered by existing envoy tests.
Part of #2557

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-05-28T23:34:12Z

/assign @wu-bin

jmarantz · 2019-05-29T13:28:21Z

test/extensions/quic_listeners/quiche/envoy_quic_alarm_test.cc

+
+protected:
+  Event::SimulatedTimeSystemHelper time_system_;
+  EnvoyQuicClock clock_;


where does this come from? I am not seeing it in the PR.

Added in #7028

jmarantz · 2019-05-29T13:37:31Z

source/extensions/quic_listeners/quiche/envoy_quic_alarm.h

+namespace Envoy {
+namespace Quic {
+
+class EnvoyQuicAlarm : public quic::QuicAlarm {


probably worth some class comments here and in other classes. I can tell you are trying to mate two different time-systems and it may be worth a whiteboard discussion on the strategy.

Added comment. The current plan is to use TimeSystem to implement QuicTime and Timer to implement QuicAlarm. These two are quite straight forward integration.

Cool. Consider using TimeSource rather than TimeSystem if possible. The only user of TimeSystem outside of tests currently is DIspatcher and we'd prefer to keep it that way if possible.

QuicClock has a ApproximateNow() which needs to interact with Dispatcher to get a timestamp in each event loop. This is not implemented yet, see TODO in EnvoyQuicClock.
TimeSystem has createScheduler() which allows us to plumb through to pass around that timestamp. So I use TimeSystem for QuicTime.

And EnvoyQuicClock will be instantiated with the TimeSystem in Dispatcher.

what's the semantics of ApproximateNow()? How would it interact with the dispatcher? Might be worth a quick write-up to explain your plans. Maybe discuss in the context of a github issue, and reference that issue here in a code comment?

opened #7177. Since alarm is not directly related to QuicClock, I didn't reference the issue here. I think the original TODO in QuicClock is sufficient.

jmarantz · 2019-05-29T13:43:52Z

include/envoy/event/timer.h

-  virtual void enableTimer(const std::chrono::milliseconds& d) PURE;
+  virtual void enableTimerInUs(const std::chrono::microseconds& d) PURE;
+
+  virtual void enableTimer(const std::chrono::milliseconds& d) {


I see what you are trying to do but I have a slightly different suggestion. See test/test_common/test_time_system.h and the definitions of sleep() for the pattern. It references TimeSystem::Duration which is defined in include/envoy/event/timer.h, and does not tie the caller to any particular unit.

That way it can be up to the caller whether to specify milliseconds or microseconds or whatever.

Are you suggesting adding another member function like void enableTimerAfterDuration(Duration d)?

Yes, but I was suggesting naming it enableTimer(Duration d) as an inline template method, to avoid having to change any existing callsites.

Look at the pattern in test_time_system.h.

How can template method be virtual?

Sorry, I misunderstood your suggestion. Please ignore my previous question.
You mean change Timer interface to be something as below right?

virtual void enableTimer(const MonotonicTime::duration& d) PURE;
template void enableTimer(const D& duration) {
enableTimer(std::chrono::duration_castMonotonicTime::duration(duration));
}

But doing so still requires changes to all EXPECT_CALL(timer, enableTime(some_duration)). I don't have strong preference to this template approach v.s. adding enableTimerInUs(). If the refactor takes some large scope change, I would prefer the former one actually.

The difference is that we still keep the old enableTimer(const std::chrono::milliseconds&) there. Is that ok?

Yeah I think that'd be OK if it works. I've had difficulties in the past overriding one of two overloaded virtual methods, which I think is what we would happen with mocks. It's worth trying.

Anyway I wind to wind up in a state where there's one enableTimer entry that takes a Duration rather than having variants for different time units. It might be simplest to start a separate PR that makes that change, including some boring changes to mocks.

+1 to single enableTimer taking a Duration. It seems to have higher resolution than milliseconds and low resolution duration types(e.g. std::chrono::seconds) can implicitly convert to it.

Agree to do it in separate PR. Filed #6588 for tracking.

I talked to Bin and Ian offline, having milliseconds granularity is fine for now. In QUICHE code, QuicAlarm has always been scheduled with (Now + some milliseconds) as RTT is usually in ms granularity. The only exception comes when it is scheduled with Now in which case casting now to milliseconds doesn't change the expected behavior. So I will use existing enbableTimer() for now.

source/extensions/quic_listeners/quiche/envoy_quic_alarm.cc

jmarantz · 2019-05-29T13:46:20Z

source/extensions/quic_listeners/quiche/envoy_quic_alarm_factory.h

+namespace Envoy {
+namespace Quic {
+
+class EnvoyQuicAlarmFactory : public quic::QuicAlarmFactory {


inherit from NonCopyable.

include/envoy/event/timer.h

wu-bin · 2019-05-29T12:59:04Z

source/extensions/quic_listeners/quiche/envoy_quic_alarm.cc

+  timer_->enableTimerInUs(std::chrono::microseconds(getDurationBeforeDeadline().ToMicroseconds()));
+}
+
+void EnvoyQuicAlarm::UpdateImpl() { SetImpl(); }


If an alarm is Set() at time_1 and Update() at time_2, will it fire twice in both times?

In any case, it's not obvious that simply calling SetImpl() will cancel the previously set alarm, a comment will be helpful.

source/extensions/quic_listeners/quiche/envoy_quic_alarm.h

wu-bin · 2019-05-29T14:08:49Z

test/extensions/quic_listeners/quiche/envoy_quic_alarm_test.cc

+  alarm1->Set(clock_.Now() + QuicTime::Delta::FromMicroseconds(10));
+  EXPECT_TRUE(alarm1->IsSet());
+  auto unowned_delegate2 = new TestDelegate();
+  quic::QuicArenaScopedPtr<quic::QuicAlarm> alarm2(alarm_factory_.CreateAlarm(unowned_delegate2));


nit: alarm2 not needed for this test.

alarm2 is needed to verify that a not-cancelled alarm should have fired.

wu-bin · 2019-05-29T14:09:10Z

test/extensions/quic_listeners/quiche/envoy_quic_alarm_test.cc

+  quic::QuicArenaScopedPtr<quic::QuicAlarm> alarm1(alarm_factory_.CreateAlarm(unowned_delegate1));
+  alarm1->Set(clock_.Now() + QuicTime::Delta::FromMicroseconds(10));
+  auto unowned_delegate2 = new TestDelegate();
+  quic::QuicArenaScopedPtr<quic::QuicAlarm> alarm2(alarm_factory_.CreateAlarm(unowned_delegate2));


nit: alarm2 not needed for this test.

alarm2 is needed to verify that a not-reset alarm shouldn't have fired.

I think the behavior of alarm2 in this test is the same as in the previous test, if you separate it out then it can be shared. But again your call.

test/extensions/quic_listeners/quiche/envoy_quic_alarm_test.cc

wu-bin · 2019-05-29T14:14:51Z

test/extensions/quic_listeners/quiche/envoy_quic_alarm_test.cc

+  bool fired_;
+};
+
+class EnvoyQuicAlarmTest : public ::testing::Test {


Some alarm library "eagerly fires" when a alarm deadline is <= now. It seems like this implementation doesn't do that, can you add a test to ensure that behavior?

What does "eagerly fire" mean? Current implementation do schedule the alarm in same event loop. Added test FireAlarmWithPastDeadline

Thanks for adding the test:)

By "eagerly fire" I mean if you schedule an alarm to fire "in the past", the schedule api will call the Fire() callback before it returns, for such alarms we need to make sure the callback is safe to run in both the calling thread and the event loop.

Signed-off-by: Dan Zhang <danzh@google.com>

include/envoy/event/timer.h

wu-bin · 2019-05-30T12:34:58Z

include/envoy/event/timer.h

-  virtual void enableTimer(const std::chrono::milliseconds& d) PURE;
+  virtual void enableTimerInUs(const std::chrono::microseconds& d) PURE;
+
+  virtual void enableTimer(const std::chrono::milliseconds& d) {


I think changing all EXPECT_CALLs is fine, but I'm not a big fan of using the same name for the virtual and template functions, it just feel very wired.

Long term, I'd suggest we use a duration type that does not imply the unit of time, like absl::Duration, for the enableTimer interface, but it's out of the scope of this PR.

wu-bin · 2019-05-30T12:43:37Z

test/extensions/quic_listeners/quiche/envoy_quic_alarm_test.cc

+  bool fired_;
+};
+
+class EnvoyQuicAlarmTest : public ::testing::Test {


Thanks for adding the test:)

By "eagerly fire" I mean if you schedule an alarm to fire "in the past", the schedule api will call the Fire() callback before it returns, for such alarms we need to make sure the callback is safe to run in both the calling thread and the event loop.

wu-bin · 2019-05-30T12:47:25Z

test/extensions/quic_listeners/quiche/envoy_quic_alarm_test.cc

+  quic::QuicArenaScopedPtr<quic::QuicAlarm> alarm1(alarm_factory_.CreateAlarm(unowned_delegate1));
+  alarm1->Set(clock_.Now() + QuicTime::Delta::FromMicroseconds(10));
+  auto unowned_delegate2 = new TestDelegate();
+  quic::QuicArenaScopedPtr<quic::QuicAlarm> alarm2(alarm_factory_.CreateAlarm(unowned_delegate2));


I think the behavior of alarm2 in this test is the same as in the previous test, if you separate it out then it can be shared. But again your call.

wu-bin · 2019-05-30T12:48:41Z

test/extensions/quic_listeners/quiche/envoy_quic_alarm_test.cc

+  auto unowned_delegate = new TestDelegate();
+  quic::QuicArenaScopedPtr<quic::QuicAlarm> alarm(alarm_factory_.CreateAlarm(unowned_delegate));
+  alarm->Set(clock_.Now() - QuicTime::Delta::FromMicroseconds(10));
+  base_scheduler_.run(Dispatcher::RunType::NonBlock);


Can you ensure that fired() is false before base_scheduler_.run?

test/extensions/quic_listeners/quiche/envoy_quic_alarm_test.cc

wu-bin · 2019-05-30T12:54:55Z

test/extensions/quic_listeners/quiche/envoy_quic_alarm_test.cc

+  EXPECT_TRUE(unowned_delegate->fired());
+}
+
+TEST_F(EnvoyQuicAlarmTest, FireAlarmWithPastDeadline) {


This test is identical to SetAlarmToPastTime. Maybe change it to UpdateAlarmToPastTime and test that Update() to a past time will fire the alarm immediately?

wu-bin · 2019-05-30T12:55:05Z

test/extensions/quic_listeners/quiche/envoy_quic_alarm_test.cc

+  quic::QuicArenaScopedPtr<quic::QuicAlarm> alarm(alarm_factory_.CreateAlarm(unowned_delegate));
+  // alarm becomes active upon Set().
+  alarm->Set(clock_.Now() - QuicTime::Delta::FromMicroseconds(10));
+  base_scheduler_.run(Dispatcher::RunType::NonBlock);


Can you ensure that fired() is false before base_scheduler_.run?

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-06-06T16:18:57Z

Ping? @wu-bin @jmarantz

wu-bin

Thanks for separating out the timer resolution issue in #7170. This PR is much easier to review now:)

wu-bin · 2019-06-06T17:15:50Z

source/extensions/quic_listeners/quiche/envoy_quic_alarm.h

+  EnvoyQuicAlarm(Event::Scheduler& scheduler, quic::QuicClock& clock,
+                 quic::QuicArenaScopedPtr<quic::QuicAlarm::Delegate> delegate);
+
+  ~EnvoyQuicAlarm() override{};


nit: Can we check that IsSet() is false in destructor?

Signed-off-by: Dan Zhang <danzh@google.com>

jmarantz

nice. I'm not sure I fully understand the semantics of the Quic library you are mating too, so I'm good with @wu-bin's review there. Envoy side LGTM.

jmarantz · 2019-06-07T00:11:56Z

source/extensions/quic_listeners/quiche/envoy_quic_alarm.cc

+EnvoyQuicAlarm::EnvoyQuicAlarm(Event::Scheduler& scheduler, quic::QuicClock& clock,
+                               quic::QuicArenaScopedPtr<quic::QuicAlarm::Delegate> delegate)
+    : QuicAlarm(std::move(delegate)), scheduler_(scheduler), clock_(clock) {
+  timer_ = scheduler_.createTimer([this]() { Fire(); });


nit: use constructor initializer.

jmarantz · 2019-06-07T00:12:40Z

source/extensions/quic_listeners/quiche/envoy_quic_alarm.cc

+void EnvoyQuicAlarm::CancelImpl() { timer_->disableTimer(); }
+
+void EnvoyQuicAlarm::SetImpl() {
+  // TODO switch to use microseconds after issue #7170 is addressed.


nit: TODO(#7170)

[fixed typo -> 7180 --> 7170]

jmarantz · 2019-06-07T00:15:13Z

test/extensions/quic_listeners/quiche/envoy_quic_alarm_test.cc

+  TestDelegate() : fired_(false) {}
+
+  void OnAlarm() override { fired_ = true; }
+  bool fired() const { return fired_; }


is this also an override?

fired() is not.

jmarantz · 2019-06-07T00:19:30Z

@envoyproxy/senior-maintainers over to you.

mattklein123

LGTM other than remaining @jmarantz nits.

/wait

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-06-07T18:52:47Z

/retest

repokitteh-read-only · 2019-06-07T18:52:51Z

🔨 rebuilding ci/circleci: tsan (failed build)

🐱

Caused by: a #7094 (comment) was created by @danzh2010.

see: more, trace.

danzh2010 · 2019-06-07T20:04:09Z

/retest

repokitteh-read-only · 2019-06-07T20:04:14Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #7094 (comment) was created by @danzh2010.

see: more, trace.

mattklein123

Thank you!

lizan · 2019-06-07T21:48:55Z

source/extensions/quic_listeners/quiche/envoy_quic_alarm_factory.h

+#pragma once
+
+// QUICHE allows unused parameters.
+#pragma GCC diagnostic ignored "-Wunused-parameter"


Can you do GCC diagnostic push / pop respectively before and after quiche imports? So the check still applies to envoy code base.

Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123

Thanks

danzh2010 requested review from alyssawilk and mattklein123 as code owners May 28, 2019 23:23

Add EnvoyQuicAlarm, EnvoyQuicAlarmFactory impl

c1e7e36

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 force-pushed the envoyquicalarm branch from b99c3ae to c1e7e36 Compare May 28, 2019 23:28

danzh2010 requested review from snowp and zuercher as code owners May 28, 2019 23:28

Merge branch 'master' into envoyquicalarm

90a082d

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot assigned wu-bin May 28, 2019

jmarantz reviewed May 29, 2019

View reviewed changes

jmarantz self-assigned this May 29, 2019

wu-bin reviewed May 29, 2019

View reviewed changes

address comment

6b2fc8c

Signed-off-by: Dan Zhang <danzh@google.com>

wu-bin reviewed May 30, 2019

View reviewed changes

danzh1989 added 3 commits May 30, 2019 12:34

refine test

9e7338c

Signed-off-by: Dan Zhang <danzh@google.com>

revert Timer change

36029bb

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into envoyquicalarm

ab2c5e0

Signed-off-by: Dan Zhang <danzh@google.com>

wu-bin previously approved these changes Jun 6, 2019

View reviewed changes

check cancel before destruction

4b3d31d

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 dismissed wu-bin’s stale review via 4b3d31d June 6, 2019 20:26

jmarantz previously approved these changes Jun 7, 2019

View reviewed changes

mattklein123 requested changes Jun 7, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 7, 2019

mattklein123 self-assigned this Jun 7, 2019

address comment

01dc600

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 dismissed jmarantz’s stale review via 01dc600 June 7, 2019 18:08

repokitteh-read-only bot removed the waiting label Jun 7, 2019

mattklein123 previously approved these changes Jun 7, 2019

View reviewed changes

lizan reviewed Jun 7, 2019

View reviewed changes

add #pragma GCC disagnostic push/pop

b8788c0

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 dismissed mattklein123’s stale review via b8788c0 June 10, 2019 16:22

mattklein123 approved these changes Jun 10, 2019

View reviewed changes

mattklein123 merged commit 68020a3 into envoyproxy:master Jun 10, 2019

quiche:: Implement QuicAlarm interface #7094

quiche:: Implement QuicAlarm interface #7094

Conversation

danzh2010 commented May 28, 2019 • edited

danzh2010 commented May 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danzh2010 commented Jun 6, 2019

wu-bin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz Jun 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz commented Jun 7, 2019

mattklein123 left a comment

Choose a reason for hiding this comment

danzh2010 commented Jun 7, 2019

repokitteh-read-only bot commented Jun 7, 2019

danzh2010 commented Jun 7, 2019

repokitteh-read-only bot commented Jun 7, 2019

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

danzh2010 commented May 28, 2019 •

edited

jmarantz Jun 7, 2019 •

edited