Conversation
5f1c0e0 to
10f2ce0
Compare
| #include <aws/core/utils/HashingUtils.h> | ||
| #include <aws/core/utils/base64/Base64.h> | ||
| #include <aws/core/utils/crypto/CRC64.h> | ||
| #include <aws/core/utils/memory/stl/AWSArray.h> |
There was a problem hiding this comment.
nit: include but not used
There was a problem hiding this comment.
it is used, the DefaultEndpointProvider included this transitively, now it doesnt, so where ever we are using it, we need to to include it now, i.e. same in transfer manager
| ClientContextParametersT> { | ||
| public: | ||
| CrtEndpointProvider(const char *endpointRulesBlob, const size_t endpointRulesBlobSz) | ||
| : m_crtEngine(Aws::Crt::ByteCursorFromArray((const uint8_t *) endpointRulesBlob, endpointRulesBlobSz), |
There was a problem hiding this comment.
One thing i wanna point out here since we are wrapping the raw pointer endpointRulesBlob in a ByteCursor (non-owning). The two engines slightly differ in construction:
- RuleEngine's constructor calls
aws_endpoints_ruleset_new_from_stringwhich parses the JSON into it's internal DOM (copy) - BddEngine's constructor calls
aws_endpoints_bdd_engine_new_from_bytecodeon the partitions blob (copy), but explicitly states that the caller is responsible for keeping the bytecode buffer alive
For the BDD path, should we ensure the lifetime of that cursor by copying it into a Aws::Vector<uint8_t> or Aws::String member member variable before constructing?
ex.
BDDEndpointProvider(const char* endpointRulesBlob, const size_t endpointRulesBlobSz)
// Copy the bytecode into a member first
: m_ownedBytecode(reinterpret_cast<const uint8_t*>(endpointRulesBlob),
reinterpret_cast<const uint8_t*>(endpointRulesBlob) + endpointRulesBlobSz),
Base(reinterpret_cast<const char*>(m_ownedBytecode.data()), m_ownedBytecode.size())
There was a problem hiding this comment.
good callout, and we solve this through our constexpr ruleset blob, that puts the data firmly in .rodata so we never have to worry about lifetimes. Lets address this the same as CRT and add a loud warning comment. Anyone who will newly subclass this for their own endpoint provider will have to do so explicitly the only path they would get it is if they didnt hav a custom endpoint provider, so it would be a new code path. Lets add a warning to match CRT.
4 participants
Description of changes:
Binds out the CRT BDD endpoint engine into the SDK. refactors common components between different CRT endpoint provider implementations behind a template.
Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.