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
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
c1e7e36
Add EnvoyQuicAlarm, EnvoyQuicAlarmFactory impl
danzh1989 90a082d
Merge branch 'master' into envoyquicalarm
danzh1989 6b2fc8c
address comment
danzh1989 9e7338c
refine test
danzh1989 36029bb
revert Timer change
danzh1989 ab2c5e0
Merge branch 'master' into envoyquicalarm
danzh1989 4b3d31d
check cancel before destruction
danzh1989 01dc600
address comment
danzh1989 b8788c0
add #pragma GCC disagnostic push/pop
danzh1989 File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
35 changes: 35 additions & 0 deletions
35
source/extensions/quic_listeners/quiche/envoy_quic_alarm.cc
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
#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), | ||
timer_(scheduler_.createTimer([this]() { Fire(); })), clock_(clock) {} | ||
|
||
void EnvoyQuicAlarm::CancelImpl() { timer_->disableTimer(); } | ||
|
||
void EnvoyQuicAlarm::SetImpl() { | ||
// TODO(#7170) switch to use microseconds if it is supported. | ||
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 |
39 changes: 39 additions & 0 deletions
39
source/extensions/quic_listeners/quiche/envoy_quic_alarm.h
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 { | ||
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 |
22 changes: 22 additions & 0 deletions
22
source/extensions/quic_listeners/quiche/envoy_quic_alarm_factory.cc
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 |
39 changes: 39 additions & 0 deletions
39
source/extensions/quic_listeners/quiche/envoy_quic_alarm_factory.h
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
#pragma once | ||
|
||
#include "common/common/non_copyable.h" | ||
|
||
#include "extensions/quic_listeners/quiche/envoy_quic_alarm.h" | ||
|
||
#pragma GCC diagnostic push | ||
|
||
// QUICHE allows unused parameters. | ||
#pragma GCC diagnostic ignored "-Wunused-parameter" | ||
#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" | ||
|
||
#pragma GCC diagnostic pop | ||
|
||
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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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 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.
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.
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 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.
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.
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 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.