Conversation
…n windows for some reason so, gonna push it up so i can run them from my mac.
…e sure the client actually works.
SergeyRyabinin
left a comment
SergeyRyabinin
left a comment
There was a problem hiding this comment.
Overall looks good.
CRT submodule is checked-out to a simple commit hash not the hash of a released tag.
Also, a corresponding .sh script for customers using git without sub-modules is not updated.
You can use the following script to auto-pull latest released CRT (warning, it will reset/checkout . changes in your local /crt):
| requestOptions.onIncomingBody = | ||
| [response](Crt::Http::HttpStream&, const Crt::ByteCursor& body) | ||
| { | ||
| response->GetResponseBody().write((const char*)body.ptr, static_cast<long>(body.len)); |
There was a problem hiding this comment.
do we need to check if the response body's stream has gone bad while writing?
docs talk about goodbit and badbit
There was a problem hiding this comment.
As I read through this, I went back and looked at all we do in the current libcurl client read callback. I need to rework the input stream section as well as this, and it's probably going to be big enough I have to break this out into another function. Will handle it in that refactor.
There was a problem hiding this comment.
We likely should. Both existing clients do not, so for the moment, I'd prefer not to alter anything we don't have to.
…in all the extra edge cases I found in the curl client implementation.
graebm
left a comment
graebm
left a comment
There was a problem hiding this comment.
still reviewing, but here's what I've got at the end of today...
tests/aws-cpp-sdk-transcribestreaming-integration-tests/TranscribeTests.cpp
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| // initial check to see if we should avoid reading for the moment. | ||
| if (!m_rateLimiter || (m_rateLimiter && m_rateLimiter->ApplyCost(0) == std::chrono::milliseconds(0))) { |
There was a problem hiding this comment.
The CRT HTTP client is going to spin hot waiting for the rate limiter. Maybe should should sleep the thread?
There was a problem hiding this comment.
this is getting called from an aws-c-io event loop though?
| // this is safe because of the isAwsChunked branch above. | ||
| // I don't see a aws_byte_buf equivalent of memmove. This is lifted from the curl implementation. | ||
| memmove(originalBufferPos + hex.size() + 2, originalBufferPos, amountRead); | ||
| memmove(originalBufferPos + hex.size() + 2 + amountRead, "\r\n", 2); | ||
| memmove(originalBufferPos, hex.c_str(), hex.size()); | ||
| memmove(originalBufferPos + hex.size(), "\r\n", 2); |
There was a problem hiding this comment.
debatable: this aws-chunked stuff is a lot of copy/pasted finicky code. Consider a utility function that both the CurlHttpClient and CRTHttpClient can share
| Aws::String hex = Aws::Utils::StringUtils::ToHexString(amountRead); | ||
| // this is safe because of the isAwsChunked branch above. | ||
| // I don't see a aws_byte_buf equivalent of memmove. This is lifted from the curl implementation. | ||
| memmove(originalBufferPos + hex.size() + 2, originalBufferPos, amountRead); |
There was a problem hiding this comment.
We could skip a copy, and maybe have simpler code, if we knew in advance how long the aws-chunk prefix was going to be.
And we'd know the length if we wrote a fixed-length hex string
There was a problem hiding this comment.
? I don't know what you're asking but if it isn't altering the existing chunked encoding behavior at all, I'm not really interested in spending more time optimizing out a copy at the moment.
jmklix
added
the
needs-review
|
Merged here: #2376 |
4 participants
Added the CRT's HTTP client as an HTTP implementation. It's currently defalted to off, so this allows experimentation without disrupting any current defaults.
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.