Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

quiche:: Implement QuicAlarm interface #7094

Merged
merged 9 commits into from Jun 10, 2019
Merged

quiche:: Implement QuicAlarm interface #7094

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c1e7e36
Add EnvoyQuicAlarm, EnvoyQuicAlarmFactory impl
danzh1989 May 24, 2019
90a082d
Merge branch 'master' into envoyquicalarm
danzh1989 May 28, 2019
6b2fc8c
address comment
danzh1989 May 29, 2019
9e7338c
refine test
danzh1989 May 30, 2019
36029bb
revert Timer change
danzh1989 Jun 5, 2019
ab2c5e0
Merge branch 'master' into envoyquicalarm
danzh1989 Jun 5, 2019
4b3d31d
check cancel before destruction
danzh1989 Jun 6, 2019
01dc600
address comment
danzh1989 Jun 7, 2019
b8788c0
add #pragma GCC disagnostic push/pop
danzh1989 Jun 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 43 additions & 0 deletions bazel/external/quiche.BUILD
View file
Open in desktop
Expand Up @@ -349,6 +349,37 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "quic_core_arena_scoped_ptr_lib",
hdrs = ["quiche/quic/core/quic_arena_scoped_ptr.h"],
repository = "@envoy",
visibility = ["//visibility:public"],
deps = [":quic_platform_base"],
)

envoy_cc_library(
name = "quic_core_alarm_lib",
srcs = ["quiche/quic/core/quic_alarm.cc"],
hdrs = ["quiche/quic/core/quic_alarm.h"],
repository = "@envoy",
visibility = ["//visibility:public"],
deps = [
":quic_core_arena_scoped_ptr_lib",
":quic_core_time_lib",
],
)

envoy_cc_library(
name = "quic_core_alarm_factory_lib",
hdrs = ["quiche/quic/core/quic_alarm_factory.h"],
repository = "@envoy",
visibility = ["//visibility:public"],
deps = [
":quic_core_alarm_lib",
":quic_core_one_block_arena_lib",
],
)

envoy_cc_library(
name = "quic_core_buffer_allocator_lib",
srcs = [
Expand All @@ -374,6 +405,18 @@ envoy_cc_library(
deps = [":quic_platform_export"],
)

envoy_cc_library(
name = "quic_core_one_block_arena_lib",
srcs = ["quiche/quic/core/quic_one_block_arena.h"],
repository = "@envoy",
visibility = ["//visibility:public"],
deps = [
":quic_core_arena_scoped_ptr_lib",
":quic_core_types_lib",
":quic_platform_base",
],
)

envoy_cc_library(
name = "quic_core_time_lib",
srcs = ["quiche/quic/core/quic_time.cc"],
Expand Down
7 changes: 6 additions & 1 deletion include/envoy/event/timer.h
View file
Open in desktop
Expand Up @@ -30,7 +30,12 @@ class Timer {
/**
* Enable a pending timeout. If a timeout is already pending, it will be reset to the new timeout.
*/
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) {
danzh2010 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can template method be virtual?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// TODO(#4332): use duration_cast more nicely to clean up this code.
enableTimerInUs(std::chrono::duration_cast<std::chrono::microseconds>(d));
}

/**
* Return whether the timer is currently armed.
Expand Down
8 changes: 3 additions & 5 deletions source/common/event/timer_impl.cc
View file
Open in desktop
Expand Up @@ -18,15 +18,13 @@ TimerImpl::TimerImpl(Libevent::BasePtr& libevent, TimerCb cb) : cb_(cb) {

void TimerImpl::disableTimer() { event_del(&raw_event_); }

void TimerImpl::enableTimer(const std::chrono::milliseconds& d) {
void TimerImpl::enableTimerInUs(const std::chrono::microseconds& d) {
if (d.count() == 0) {
event_active(&raw_event_, EV_TIMEOUT, 0);
} else {
// TODO(#4332): use duration_cast more nicely to clean up this code.
std::chrono::microseconds us = std::chrono::duration_cast<std::chrono::microseconds>(d);
timeval tv;
tv.tv_sec = us.count() / 1000000;
tv.tv_usec = us.count() % 1000000;
tv.tv_sec = d.count() / 1000000;
tv.tv_usec = d.count() % 1000000;
event_add(&raw_event_, &tv);
}
}
Expand Down
2 changes: 1 addition & 1 deletion source/common/event/timer_impl.h
View file
Open in desktop
Expand Up @@ -19,7 +19,7 @@ class TimerImpl : public Timer, ImplBase {

// Timer
void disableTimer() override;
void enableTimer(const std::chrono::milliseconds& d) override;
void enableTimerInUs(const std::chrono::microseconds& d) override;
bool enabled() override;

private:
Expand Down
27 changes: 21 additions & 6 deletions source/extensions/quic_listeners/quiche/BUILD
View file
Open in desktop
Expand Up @@ -8,11 +8,26 @@ load(

envoy_package()

# Placeholder library to verify/illustrate depending on a QUICHE build target.
# TODO(mpwarres): remove once real build rules added here.
envoy_cc_library(
name = "dummy_lib",
srcs = ["dummy.cc"],
hdrs = ["dummy.h"],
external_deps = ["quiche_http2_platform"],
name = "envoy_quic_alarm_lib",
srcs = ["envoy_quic_alarm.cc"],
hdrs = ["envoy_quic_alarm.h"],
external_deps = ["quiche_quic_platform"],
deps = [
"//include/envoy/event:timer_interface",
"@com_googlesource_quiche//:quic_core_alarm_lib",
],
)

envoy_cc_library(
name = "envoy_quic_alarm_factory_lib",
srcs = ["envoy_quic_alarm_factory.cc"],
hdrs = ["envoy_quic_alarm_factory.h"],
external_deps = ["quiche_quic_platform"],
deps = [
":envoy_quic_alarm_lib",
"@com_googlesource_quiche//:quic_core_alarm_factory_lib",
"@com_googlesource_quiche//:quic_core_arena_scoped_ptr_lib",
"@com_googlesource_quiche//:quic_core_one_block_arena_lib",
],
)
30 changes: 30 additions & 0 deletions source/extensions/quic_listeners/quiche/envoy_quic_alarm.cc
View file
Open in desktop
@@ -0,0 +1,30 @@
#include "extensions/quic_listeners/quiche/envoy_quic_alarm.h"

namespace Envoy {
namespace Quic {

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(); });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: use constructor initializer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}

void EnvoyQuicAlarm::CancelImpl() { timer_->disableTimer(); }

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

void EnvoyQuicAlarm::UpdateImpl() { SetImpl(); }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


quic::QuicTime::Delta EnvoyQuicAlarm::getDurationBeforeDeadline() {
quic::QuicTime::Delta duration(quic::QuicTime::Delta::Zero());
quic::QuicTime now = clock_.ApproximateNow();
if (deadline() > now) {
danzh2010 marked this conversation as resolved.
Show resolved Hide resolved
duration = deadline() - now;
}
return duration;
}

} // namespace Quic
} // namespace Envoy
34 changes: 34 additions & 0 deletions source/extensions/quic_listeners/quiche/envoy_quic_alarm.h
View file
Open in desktop
@@ -0,0 +1,34 @@
#pragma once

#include "envoy/event/timer.h"

#include "quiche/quic/core/quic_alarm.h"
#include "quiche/quic/core/quic_time.h"
#include "quiche/quic/platform/api/quic_clock.h"

namespace Envoy {
namespace Quic {

class EnvoyQuicAlarm : public quic::QuicAlarm {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

public:
EnvoyQuicAlarm(Event::Scheduler& scheduler, quic::QuicClock& clock,
quic::QuicArenaScopedPtr<quic::QuicAlarm::Delegate> delegate);

~EnvoyQuicAlarm() override{};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


// quic::QuicAlarm
void CancelImpl() override;
void SetImpl() override;
// Overridden to avoid cancel before set.
void UpdateImpl() override;

private:
quic::QuicTime::Delta getDurationBeforeDeadline();

Event::Scheduler& scheduler_;
Event::TimerPtr timer_{nullptr};
danzh2010 marked this conversation as resolved.
Show resolved Hide resolved
quic::QuicClock& clock_;
};

} // namespace Quic
} // namespace Envoy
22 changes: 22 additions & 0 deletions source/extensions/quic_listeners/quiche/envoy_quic_alarm_factory.cc
View file
Open in desktop
@@ -0,0 +1,22 @@
#include "extensions/quic_listeners/quiche/envoy_quic_alarm_factory.h"

namespace Envoy {
namespace Quic {

quic::QuicAlarm* EnvoyQuicAlarmFactory::CreateAlarm(quic::QuicAlarm::Delegate* delegate) {
return new EnvoyQuicAlarm(scheduler_, clock_,
quic::QuicArenaScopedPtr<quic::QuicAlarm::Delegate>(delegate));
}

quic::QuicArenaScopedPtr<quic::QuicAlarm>
EnvoyQuicAlarmFactory::CreateAlarm(quic::QuicArenaScopedPtr<quic::QuicAlarm::Delegate> delegate,
quic::QuicConnectionArena* arena) {
if (arena != nullptr) {
return arena->New<EnvoyQuicAlarm>(scheduler_, clock_, std::move(delegate));
}
return quic::QuicArenaScopedPtr<quic::QuicAlarm>(
new EnvoyQuicAlarm(scheduler_, clock_, std::move(delegate)));
}

} // namespace Quic
} // namespace Envoy
39 changes: 39 additions & 0 deletions source/extensions/quic_listeners/quiche/envoy_quic_alarm_factory.h
View file
Open in desktop
@@ -0,0 +1,39 @@
#pragma once

// QUICHE allows unused parameters.
#pragma GCC diagnostic ignored "-Wunused-parameter"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


#include "extensions/quic_listeners/quiche/envoy_quic_alarm.h"

#include "quiche/quic/core/quic_alarm.h"
#include "quiche/quic/core/quic_alarm_factory.h"
#include "quiche/quic/core/quic_arena_scoped_ptr.h"
#include "quiche/quic/core/quic_one_block_arena.h"

namespace Envoy {
namespace Quic {

class EnvoyQuicAlarmFactory : public quic::QuicAlarmFactory {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inherit from NonCopyable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

public:
EnvoyQuicAlarmFactory(Event::Scheduler& scheduler, quic::QuicClock& clock)
: scheduler_(scheduler), clock_(clock) {}

EnvoyQuicAlarmFactory(const EnvoyQuicAlarmFactory&) = delete;

~EnvoyQuicAlarmFactory() override {}

EnvoyQuicAlarmFactory& operator=(const EnvoyQuicAlarmFactory) = delete;

// QuicAlarmFactory
quic::QuicAlarm* CreateAlarm(quic::QuicAlarm::Delegate* delegate) override;
quic::QuicArenaScopedPtr<quic::QuicAlarm>
CreateAlarm(quic::QuicArenaScopedPtr<quic::QuicAlarm::Delegate> delegate,
quic::QuicConnectionArena* arena) override;

private:
Event::Scheduler& scheduler_;
quic::QuicClock& clock_;
};

} // namespace Quic
} // namespace Envoy
12 changes: 7 additions & 5 deletions test/extensions/quic_listeners/quiche/BUILD
View file
Open in desktop
Expand Up @@ -13,11 +13,13 @@ load(
envoy_package()

envoy_cc_test(
name = "dummy_test",
srcs = ["dummy_test.cc"],
external_deps = ["quiche_http2_platform"],
name = "envoy_quic_alarm_test",
srcs = ["envoy_quic_alarm_test.cc"],
external_deps = ["quiche_quic_platform"],
deps = [
"//source/extensions/quic_listeners/quiche:dummy_lib",
"//test/test_common:utility_lib",
"//source/extensions/quic_listeners/quiche:envoy_quic_alarm_factory_lib",
"//source/extensions/quic_listeners/quiche:envoy_quic_alarm_lib",
"//source/extensions/quic_listeners/quiche/platform:envoy_quic_clock_lib",
"//test/test_common:simulated_time_system_lib",
],
)