github/azure-sdk-for-cpp
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

Changes around context expiration naming and types (#2033)

Browse Source 
This would unblock #2010.

Closes #2032.
Closes #1880.

I know this comes out of the blue, but it seems to me the right change to do. This is a proposal, let me know what you think.

First, I started with the fact that we need to make `CancelWhen()` public.
Then, I realized that I don't like the `CancelWhen()` naming. `CancelAt()` is better?
But `CancelAt()` sounds like an order to do something, not as getter.
So, it should be named `Get...`. `GetCancelWhen()`? `GetCancelAt()`? Sounds strage.

`GetDeadline()` - not bad, bit it is not that clear, what the deadline is.
And I looked at the `WithDeadline()` method. And in comment, the first line is `@brief Create a context with expiration.`.
"Expiration" sounds better explanation for the essense of the "deadline".

So, I went with `GetExpiration()`. I think "Expiration" is also a better name, because should we want to add the method called "bool IsExpired()", it comes naturally using the existing terminology, sounds better than "`IsPastDeadline()`".

Next thing - if we have "`GetExpiration()`", it is strange to have a method called `Get()`. So, we have `WithExpiration()` and `WithValue()`. So, it sounds like the getter should be called `GetValue()`. I did that rename as well.

Then, I looked at "`With...`" method naming. It is a factory method. If for getters we have `Get`, then for factory methods we should have `Create`. So, I renamed `WithExpiration()` and `WithValue()` to `CreateWithExpiration()` and `CreateWithValue()`.

Then I looked at `Context::time_point` typedef. First, in general, if we can avoid public typedefs, it is better, because we don't need to document them and worry if we broke client code when we change them.
Second, it is strange that we use `Azure::DateTime` everywhere, but not in context.

So, I replaced it with `Azure::DateTime`. It is good because it allows us to drop to/from epoch conversions (#1880), and really all that extra dance we do to cast to representation and back to datetime is the ways to stay within legal type casting limits of compiler type declaration, while in reality there isn't much that is happening in the code: `DateTime` is essentially an `int64`, and with all these conversions to epoch time, then to representation and back, compiler does not generate real code, it still operates with that only `int64`.

Why we cast to `DateTime::rep` and back at all? Context stores it as `atomic`, and I am not questioning that. You can't get `std::atomic<DateTime>` today explicitly, so we "extract" representation (`int64`) and store it as `std::atomic<DateTime::rep>`, which is the same thing as `std::atomic_int64`.

-- Update: --
1. "Expiration" => "Deadline" (Jeffrey's request)
2. Added `Context::HasExpiration()` (response to Jinming) // plus, we do have `HasValue()` so why not have `HasDeadline()`.
3. `WithDeadline()`, `WithValue` => Two overloads of `CreateChildContext()` (my own initiative).

-- Update 2: --
`CreateChildContext()` => `WithDeadline()`, `WithValue`

-- Update 3: --
Removed `HasDeadline()`
This commit is contained in:
Anton Kolesnyk 2021-04-05 21:24:40 +00:00 committed by GitHub
parent dfdaf25223
commit c5ddbbc430
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 115 additions and 72 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
sdk/core/azure-core/CHANGELOG.md
View File

@ -14,6 +14,9 @@
- `SetHeader(uint8_t const* const first, uint8_t const* const last)`.
- `GetMajorVersion`.
- `GetMinorVersion`.
- Changes to `Azure::Core::Context`:
- Renamed `Get()` to `GetValue()`.
- Changed input parameter type of `WithDeadline()` to `Azure::DateTime`.
- Removed from `Azure::Core::Http::Policies` namespace: `HttpPolicyOrder`, `TransportPolicy`, `RetryPolicy`, `RequestIdPolicy`, `TelemetryPolicy`, `BearerTokenAuthenticationPolicy`, `LogPolicy`.
- Renamed `Azure::Core::Http::RawResponse::GetBodyStream()` to `ExtractBodyStream()`.
- Introduced `Azure::Core::Context::Key` class which takes place of `std::string` used for `Azure::Core::Context` keys previously.
@ -26,6 +29,7 @@
- Added `Azure::Core::Operation<T>::GetRawResponse()`.
- Added `Azure::Core::PackageVersion`.
- Added support for logging to console when `AZURE_LOG_LEVEL` environment variable is set.
- Added `Azure::Core::Context::GetDeadline()`.
### Breaking Changes

68
sdk/core/azure-core/inc/azure/core/context.hpp
View File

@ -8,6 +8,8 @@
#pragma once
#include "azure/core/datetime.hpp"
#include <atomic>
#include <chrono>
#include <memory>
@ -35,7 +37,7 @@ namespace Azure { namespace Core {
};
/**
* @brief A context is a node within a tree that represents expiration times and key/value pairs.
* @brief A context is a node within a tree that represents deadlines and key/value pairs.
*/
class Context {
public:
@ -56,41 +58,35 @@ namespace Azure { namespace Core {
bool operator!=(Key const& other) const { return !(*this == other); }
};
/**
* @brief A type used to provide a point in time for context expiration.
*/
using time_point = std::chrono::system_clock::time_point;
private:
struct ContextSharedState
{
std::shared_ptr<ContextSharedState> Parent;
std::atomic_int64_t CancelAtMsecSinceEpoch;
std::atomic<DateTime::rep> Deadline;
Context::Key Key;
std::shared_ptr<void> Value;
const std::type_info& ValueType;
static constexpr int64_t ToMsecSinceEpoch(time_point time)
static constexpr DateTime::rep ToDateTimeRepresentation(DateTime const& dateTime)
{
return static_cast<int64_t>(
std::chrono::duration_cast<std::chrono::milliseconds>(time - time_point()).count());
return dateTime.time_since_epoch().count();
}
static constexpr time_point FromMsecSinceEpoch(int64_t msec)
static constexpr DateTime FromDateTimeRepresentation(DateTime::rep dtRepresentation)
{
return time_point() + static_cast<std::chrono::milliseconds>(msec);
return DateTime(DateTime::time_point(DateTime::duration(dtRepresentation)));
}
explicit ContextSharedState()
: CancelAtMsecSinceEpoch(ToMsecSinceEpoch((time_point::max)())), Value(nullptr),
: Deadline(ToDateTimeRepresentation((DateTime::max)())), Value(nullptr),
ValueType(typeid(std::nullptr_t))
{
}
explicit ContextSharedState(
const std::shared_ptr<ContextSharedState>& parent,
time_point cancelAt)
: Parent(parent), CancelAtMsecSinceEpoch(ToMsecSinceEpoch(cancelAt)), Value(nullptr),
DateTime const& deadline)
: Parent(parent), Deadline(ToDateTimeRepresentation(deadline)), Value(nullptr),
ValueType(typeid(std::nullptr_t))
{
}
@ -98,10 +94,10 @@ namespace Azure { namespace Core {
template <class T>
explicit ContextSharedState(
const std::shared_ptr<ContextSharedState>& parent,
time_point cancelAt,
DateTime const& deadline,
Context::Key const& key,
T value) // NOTE, should this be T&&
: Parent(parent), CancelAtMsecSinceEpoch(ToMsecSinceEpoch(cancelAt)), Key(key),
: Parent(parent), Deadline(ToDateTimeRepresentation(deadline)), Key(key),
Value(std::make_shared<T>(std::move(value))), ValueType(typeid(T))
{
}
@ -114,11 +110,9 @@ namespace Azure { namespace Core {
{
}
time_point CancelWhen() const;
public:
/**
* @brief Construct a new context with no expiration, and no value associated.
* @brief Construct a new context with no deadline, and no value associated.
*/
Context() : m_contextSharedState(std::make_shared<ContextSharedState>()) {}
@ -128,32 +122,40 @@ namespace Azure { namespace Core {
Context& operator=(const Context&) = default;
/**
* @brief Create a context with expiration.
* @brief Create a context with a deadline.
*
* @param cancelWhen A point in time after which a context expires.
* @param deadline A point in time after which a context expires.
*
* @return A child context with expiration.
* @return A child context with deadline.
*/
Context WithDeadline(time_point cancelWhen) const
Context WithDeadline(DateTime const& deadline) const
{
return Context{std::make_shared<ContextSharedState>(m_contextSharedState, cancelWhen)};
return Context{std::make_shared<ContextSharedState>(m_contextSharedState, deadline)};
}
/**
* @brief Create a context without an expiration, but with \p key and \p value associated with
* it.
* @brief Create a context without a deadline, but with \p key and \p value associated with it.
*
* @param key A key to associate with this context.
* @param value A value to associate with this context.
*
* @return A child context with no expiration and the \p key and \p value associated with it.
* @return A child context with no deadline and the \p key and \p value associated with it.
*/
template <class T> Context WithValue(Key const& key, T&& value) const
{
return Context{std::make_shared<ContextSharedState>(
m_contextSharedState, (time_point::max)(), key, std::forward<T>(value))};
m_contextSharedState, (DateTime::max)(), key, std::forward<T>(value))};
}
/**
* @brief Get a deadline associated with this context or the branch of contexts this context
* belongs to.
*
* @return A deadline associated with the context found; `Azure::DateTime::max()` value if a
* specific value can't be found.
*/
DateTime GetDeadline() const;
/**
* @brief Get a value associated with a \p key parameter within this context or the branch of
* contexts this context belongs to.
@ -163,7 +165,7 @@ namespace Azure { namespace Core {
* @return A value associated with the context found; an empty value if a specific value can't
* be found.
*/
template <class T> const T& Get(Key const& key) const
template <class T> const T& GetValue(Key const& key) const
{
for (auto ptr = m_contextSharedState; ptr; ptr = ptr->Parent)
{
@ -208,15 +210,15 @@ namespace Azure { namespace Core {
*/
void Cancel()
{
m_contextSharedState->CancelAtMsecSinceEpoch
= ContextSharedState::ToMsecSinceEpoch((time_point::min)());
m_contextSharedState->Deadline
= ContextSharedState::ToDateTimeRepresentation((DateTime::min)());
}
/**
* @brief Check if the context is cancelled.
* @return `true` if this context is cancelled, `false` otherwise.
*/
bool IsCancelled() const { return CancelWhen() < std::chrono::system_clock::now(); }
bool IsCancelled() const { return GetDeadline() < std::chrono::system_clock::now(); }
/**
* @brief Throw an exception if the context was cancelled.

13
sdk/core/azure-core/src/context.cpp
View File

@ -4,7 +4,6 @@
#include "azure/core/context.hpp"
using namespace Azure::Core;
using time_point = std::chrono::system_clock::time_point;
Context& Azure::Core::Context::GetApplicationContext()
{
@ -12,15 +11,17 @@ Context& Azure::Core::Context::GetApplicationContext()
return context;
}
time_point Azure::Core::Context::CancelWhen() const
Azure::DateTime Azure::Core::Context::GetDeadline() const
{
auto result = time_point::max();
// Contexts form a tree. Here, we walk from a node all the way back to the root in order to find
// the earliest deadline value.
auto result = DateTime::max();
for (auto ptr = m_contextSharedState; ptr; ptr = ptr->Parent)
{
auto cancelAt = ContextSharedState::FromMsecSinceEpoch(ptr->CancelAtMsecSinceEpoch);
if (result > cancelAt)
auto deadline = ContextSharedState::FromDateTimeRepresentation(ptr->Deadline);
if (result > deadline)
{
result = cancelAt;
result = deadline;
}
}

4
sdk/core/azure-core/src/http/retry_policy.cpp
View File

@ -161,7 +161,7 @@ Context inline CreateRetryContext(Context const& parent)
int retryCount = 0;
if (parent.HasKey(RetryKey))
{
retryCount = parent.Get<int>(RetryKey) + 1;
retryCount = parent.GetValue<int>(RetryKey) + 1;
}
return parent.WithValue(RetryKey, retryCount);
}
@ -178,7 +178,7 @@ int RetryPolicy::GetRetryNumber(Context const& context)
// ...
return -1;
}
return context.Get<int>(RetryKey);
return context.GetValue<int>(RetryKey);
}
std::unique_ptr<RawResponse> RetryPolicy::Send(

94
sdk/core/azure-core/test/ut/context.cpp
View File

@ -28,7 +28,7 @@ TEST(Context, BasicBool)
// New context from previous
auto c2 = context.WithValue(key, true);
auto& value = c2.Get<bool>(key);
auto& value = c2.GetValue<bool>(key);
EXPECT_TRUE(value == true);
}
@ -39,7 +39,7 @@ TEST(Context, BasicInt)
// New context from previous
auto c2 = context.WithValue(key, 123);
auto& value = c2.Get<int>(key);
auto& value = c2.GetValue<int>(key);
EXPECT_TRUE(value == 123);
}
@ -52,7 +52,7 @@ TEST(Context, BasicStdString)
// New context from previous
auto c2 = context.WithValue(key, s);
auto& value = c2.Get<std::string>(key);
auto& value = c2.GetValue<std::string>(key);
EXPECT_TRUE(value == s);
}
@ -66,7 +66,7 @@ TEST(Context, BasicChar)
// New context from previous
auto c2 = context.WithValue(key, str);
auto& value = c2.Get<const char*>(key);
auto& value = c2.GetValue<const char*>(key);
EXPECT_TRUE(value == s);
EXPECT_TRUE(value == str);
}
@ -162,13 +162,13 @@ TEST(Context, Chain)
auto c7 = c6.WithValue(key7, "7");
auto finalContext = c7.WithValue(keyFinal, "Final");
auto& valueT2 = finalContext.Get<int>(key2);
auto& valueT3 = finalContext.Get<int>(key3);
auto& valueT4 = finalContext.Get<int>(key4);
auto& valueT5 = finalContext.Get<const char*>(key5);
auto& valueT6 = finalContext.Get<const char*>(key6);
auto& valueT7 = finalContext.Get<const char*>(key7);
auto& valueT8 = finalContext.Get<const char*>(keyFinal);
auto& valueT2 = finalContext.GetValue<int>(key2);
auto& valueT3 = finalContext.GetValue<int>(key3);
auto& valueT4 = finalContext.GetValue<int>(key4);
auto& valueT5 = finalContext.GetValue<const char*>(key5);
auto& valueT6 = finalContext.GetValue<const char*>(key6);
auto& valueT7 = finalContext.GetValue<const char*>(key7);
auto& valueT8 = finalContext.GetValue<const char*>(keyFinal);
EXPECT_TRUE(valueT2 == 123);
EXPECT_TRUE(valueT3 == 456);
@ -188,8 +188,8 @@ TEST(Context, MatchingKeys)
auto c2 = context.WithValue(key, 123);
auto c3 = c2.WithValue(key, 456);
auto& valueT2 = c2.Get<int>(key);
auto& valueT3 = c3.Get<int>(key);
auto& valueT2 = c2.GetValue<int>(key);
auto& valueT3 = c3.GetValue<int>(key);
EXPECT_TRUE(valueT2 == 123);
EXPECT_TRUE(valueT3 == 456);
@ -204,7 +204,7 @@ TEST(Context, InstanceValue)
{
Context::Key const key;
auto contextP = Context::GetApplicationContext().WithValue(key, SomeStructForContext());
auto& contextValueRef = contextP.Get<SomeStructForContext>(key);
auto& contextValueRef = contextP.GetValue<SomeStructForContext>(key);
EXPECT_EQ(contextValueRef.someField, 12345);
}
@ -213,7 +213,7 @@ TEST(Context, UniquePtr)
Context::Key const key;
auto contextP
= Context::GetApplicationContext().WithValue(key, std::make_unique<SomeStructForContext>());
auto& contextValueRef = contextP.Get<std::unique_ptr<SomeStructForContext>>(key);
auto& contextValueRef = contextP.GetValue<std::unique_ptr<SomeStructForContext>>(key);
EXPECT_EQ(contextValueRef->someField, 12345);
}
@ -232,31 +232,31 @@ TEST(Context, HeapLinkIntegrity)
auto secondGeneration = firstGeneration.WithValue(b, std::string("b"));
EXPECT_TRUE(secondGeneration.HasKey(a));
EXPECT_EQ("a", secondGeneration.Get<std::string>(a));
EXPECT_EQ("a", secondGeneration.GetValue<std::string>(a));
EXPECT_TRUE(secondGeneration.HasKey(b));
EXPECT_EQ("b", secondGeneration.Get<std::string>(b));
EXPECT_EQ("b", secondGeneration.GetValue<std::string>(b));
// Now overide the generation
secondGeneration = secondGeneration.WithValue(c, std::string("c"));
EXPECT_TRUE(
secondGeneration.HasKey(a)); // Still know about first gen - The link is still in heap
EXPECT_EQ("a", secondGeneration.Get<std::string>(a));
EXPECT_EQ("a", secondGeneration.GetValue<std::string>(a));
EXPECT_TRUE(secondGeneration.HasKey(
b)); // Still knows about the initial second gen, as a shared_ptr, it is still on heap
EXPECT_EQ("b", secondGeneration.Get<std::string>(b));
EXPECT_EQ("b", secondGeneration.GetValue<std::string>(b));
EXPECT_TRUE(secondGeneration.HasKey(c)); // Check new value
EXPECT_EQ("c", secondGeneration.Get<std::string>(c));
EXPECT_EQ("c", secondGeneration.GetValue<std::string>(c));
// One more override
secondGeneration = secondGeneration.WithValue(d, std::string("d"));
EXPECT_TRUE(secondGeneration.HasKey(a));
EXPECT_EQ("a", secondGeneration.Get<std::string>(a));
EXPECT_EQ("a", secondGeneration.GetValue<std::string>(a));
EXPECT_TRUE(secondGeneration.HasKey(b));
EXPECT_EQ("b", secondGeneration.Get<std::string>(b));
EXPECT_EQ("b", secondGeneration.GetValue<std::string>(b));
EXPECT_TRUE(secondGeneration.HasKey(c));
EXPECT_EQ("c", secondGeneration.Get<std::string>(c));
EXPECT_EQ("c", secondGeneration.GetValue<std::string>(c));
EXPECT_TRUE(secondGeneration.HasKey(d));
EXPECT_EQ("d", secondGeneration.Get<std::string>(d));
EXPECT_EQ("d", secondGeneration.GetValue<std::string>(d));
// New Gen
thirdGeneration = secondGeneration.WithValue(e, std::string("e"));
@ -264,15 +264,15 @@ TEST(Context, HeapLinkIntegrity)
// Went out of scope, root and secondGeneration are destroyed. but should remain in heap for the
// third-generation since the previous geneations are still alive inside his heart <3.
EXPECT_TRUE(thirdGeneration.HasKey(a));
EXPECT_EQ("a", thirdGeneration.Get<std::string>(a));
EXPECT_EQ("a", thirdGeneration.GetValue<std::string>(a));
EXPECT_TRUE(thirdGeneration.HasKey(b));
EXPECT_EQ("b", thirdGeneration.Get<std::string>(b));
EXPECT_EQ("b", thirdGeneration.GetValue<std::string>(b));
EXPECT_TRUE(thirdGeneration.HasKey(c));
EXPECT_EQ("c", thirdGeneration.Get<std::string>(c));
EXPECT_EQ("c", thirdGeneration.GetValue<std::string>(c));
EXPECT_TRUE(thirdGeneration.HasKey(d));
EXPECT_EQ("d", thirdGeneration.Get<std::string>(d));
EXPECT_EQ("d", thirdGeneration.GetValue<std::string>(d));
EXPECT_TRUE(thirdGeneration.HasKey(e));
EXPECT_EQ("e", thirdGeneration.Get<std::string>(e));
EXPECT_EQ("e", thirdGeneration.GetValue<std::string>(e));
}
Context::Key const GlobalKey1;
@ -325,3 +325,39 @@ TEST(Context, KeyComparison)
EXPECT_NE(localKey1Copy, localKey2);
EXPECT_NE(localKey2Copy, localKey1);
}
TEST(Context, Deadline)
{
auto const deadline = Azure::DateTime(2021, 4, 1, 23, 45, 15);
Context::Key const key1;
Context::Key const key2;
{
Context ctx;
EXPECT_EQ(ctx.GetDeadline(), Azure::DateTime::max());
ctx.Cancel();
EXPECT_EQ(ctx.GetDeadline(), Azure::DateTime::min());
}
{
Context ctx;
ctx = ctx.WithDeadline(deadline);
EXPECT_EQ(ctx.GetDeadline(), deadline);
}
{
Context ctx;
auto childCtx = ctx.WithDeadline(deadline).WithValue(key1, "val").WithValue(key2, "val2");
EXPECT_EQ(childCtx.GetDeadline(), deadline);
}
{
Context ctx;
ctx.Cancel();
auto childCtx = ctx.WithDeadline(deadline).WithValue(key1, "val").WithValue(key2, "val2");
EXPECT_EQ(childCtx.GetDeadline(), Azure::DateTime::min());
}
}

2
sdk/core/azure-core/test/ut/policy.cpp
View File

@ -64,7 +64,7 @@ struct TestContextTreeIntegrity : public Azure::Core::Http::Policies::HttpPolicy
EXPECT_TRUE(ctx.HasKey(TheKey));
if (ctx.HasKey(TheKey))
{
auto value = ctx.Get<std::string>(TheKey);
auto value = ctx.GetValue<std::string>(TheKey);
EXPECT_EQ("TheValue", value);
}
return nextHttpPolicy.Send(request, ctx);

2
sdk/storage/azure-storage-common/src/storage_switch_to_secondary_policy.cpp
View File

@ -15,7 +15,7 @@ namespace Azure { namespace Storage { namespace _internal {
std::shared_ptr<bool> replicaStatus;
if (ctx.HasKey(SecondaryHostReplicaStatusKey))
{
replicaStatus = ctx.Get<std::shared_ptr<bool>>(SecondaryHostReplicaStatusKey);
replicaStatus = ctx.GetValue<std::shared_ptr<bool>>(SecondaryHostReplicaStatusKey);
}
bool considerSecondary = (request.GetMethod() == Azure::Core::Http::HttpMethod::Get