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.
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
quiche:: Implement QuicAlarm interface #7094
Changes from 7 commits
c1e7e36
90a082d
6b2fc8c
9e7338c
36029bb
ab2c5e0
4b3d31d
01dc600
b8788c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
#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(); }); | ||
} | ||
|
||
void EnvoyQuicAlarm::CancelImpl() { timer_->disableTimer(); } | ||
|
||
void EnvoyQuicAlarm::SetImpl() { | ||
// TODO switch to use microseconds after issue #7170 is addressed. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: TODO(#7170) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [fixed typo -> 7180 --> 7170] There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
timer_->enableTimer(std::chrono::milliseconds(getDurationBeforeDeadline().ToMilliseconds())); | ||
} | ||
|
||
void EnvoyQuicAlarm::UpdateImpl() { | ||
// Since Timer::enableTimer() overrides its deadline from previous calls, | ||
// there is no need to disable the timer before enabling it again. | ||
SetImpl(); | ||
} | ||
|
||
quic::QuicTime::Delta EnvoyQuicAlarm::getDurationBeforeDeadline() { | ||
quic::QuicTime::Delta duration(quic::QuicTime::Delta::Zero()); | ||
quic::QuicTime now = clock_.ApproximateNow(); | ||
quic::QuicTime tmp = deadline(); | ||
if (tmp > now) { | ||
duration = tmp - now; | ||
} | ||
return duration; | ||
} | ||
|
||
} // namespace Quic | ||
} // namespace Envoy |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
#pragma once | ||
|
||
#include "envoy/event/timer.h" | ||
|
||
#include "common/common/assert.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 { | ||
|
||
// Implements QUIC interface | ||
// https://quiche.googlesource.com/quiche/+/refs/heads/master/quic/core/quic_alarm.h This class | ||
// wraps an Event::Timer object and provide interface for QUIC to interact with the timer. | ||
class EnvoyQuicAlarm : public quic::QuicAlarm { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. And EnvoyQuicClock will be instantiated with the TimeSystem in Dispatcher. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { ASSERT(!IsSet()); }; | ||
|
||
// 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_; | ||
quic::QuicClock& clock_; | ||
}; | ||
|
||
} // namespace Quic | ||
} // namespace Envoy |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
#pragma once | ||
|
||
// QUICHE allows unused parameters. | ||
#pragma GCC diagnostic ignored "-Wunused-parameter" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
|
||
#include "common/common/non_copyable.h" | ||
|
||
#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, NonCopyable { | ||
public: | ||
EnvoyQuicAlarmFactory(Event::Scheduler& scheduler, quic::QuicClock& clock) | ||
: scheduler_(scheduler), clock_(clock) {} | ||
|
||
~EnvoyQuicAlarmFactory() override {} | ||
|
||
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: use constructor initializer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done