Unique ptr for body stream #214
Conversation
vhvb1989
requested review from
ahsonkhan,
antkmsft and
JeffreyRichter
as code owners
Jinming-Hu
force-pushed
the
unique-ptr-for-body-stream
branch
from
ba3385e to
9eed070
Compare
Jinming-Hu
force-pushed
the
unique-ptr-for-body-stream
branch
from
9eed070 to
21d18f9
Compare
| if (isContentLengthHeaderInResponse != headers.end()) | ||
| { | ||
| // Response with Content-Length | ||
| auto bodySize = atoi(headers.at("Content-Length").data()); |
There was a problem hiding this comment.
atoi cannot handle value > INT_MAX, which is 2GB-1. use std::stoull or std::stoll instead
| } | ||
|
|
||
| // Response with no Content-Length and no Transfer-Encoding header. Throw | ||
| throw; |
There was a problem hiding this comment.
This can happen, I met once just now.
We cannot just throw;, it will crash the whole program.
There was a problem hiding this comment.
it looks like in case of no Transfer-Encoding or Content-Length, we should just read until the connection is closed.
https://tools.ietf.org/html/rfc7230#section-3.3.3
- Otherwise, this is a response message without a declared message
body length, so the message body length is determined by the
number of octets received prior to the server closing the
connection.
I going to remove the throw for now.
I need to improve the reading strategies to avoid code duplication, I will make a new issue for it
There was a problem hiding this comment.
okay, just looked through the link. It seems that this HTTP1.1 is very complex and Azure Core team is re-implementating the HTTP stack from scratch.
I have no objections to that. I just feel it's a huge load of work to make it HTTP1.1 compatible and there's going to be many many glitches.
…e-sdk-for-cpp into unique-ptr-for-body-stream
| @@ -3131,7 +3131,7 @@ namespace Azure { namespace Storage { namespace Blobs { | |||
|
|
|||
| static Azure::Core::Http::Request SetAccessTierConstructRequest( | |||
| const std::string& url, | |||
| const SetAccessTierOptions& options) | |||
| SetAccessTierOptions& options) | |||
There was a problem hiding this comment.
@JinmingHu-MSFT , all options can still be const. Why did we remove it?
There was a problem hiding this comment.
If it was const, how can we move that std::unique_ptr?
There was a problem hiding this comment.
I would let body out of the options and as an extra argument for Constructing the Request, just like is it now an extra arg for some other functions.
There was a problem hiding this comment.
@vhvb1989 I'm also okay with this way.
But there's this conclusion that Azure Core team and Azure Storage team and Jeffrey all agreed on in the meeting months ago, that in protocol layer interface, there're only two parameters, url and options.
I'm not sure if we can just overturn that decision.
There was a problem hiding this comment.
yeah, I remember.
However, we didn't consider having a unique_ptr in the options (that needs to be moved).
I also find kind of weird to have a body as an option. It has a benefit for non-body requests like GET/HEAD, but we could easily use nullptr as arg.
Having cost for options IMO is the best wat to make sure options won't be mutated. So I wouldn't change that.
I would like to bring this to our meeting @gilbertw , @JeffreyRichter
As for now, let's use options without const as the last agreement.
Thank you @JinmingHu-MSFT
There was a problem hiding this comment.
@vhvb1989 Let's bring this up in the weekly meeting, so that we can make a decision together. Once we have a conclusion, i'm going to change the code. But before that, let's just keep it as it is now.
| #include <vector> | ||
|
|
||
| namespace Azure { namespace Storage { | ||
|
|
||
| class MemoryStream : public Azure::Core::Http::BodyStream { | ||
| public: | ||
| explicit MemoryStream(const uint8_t* data, std::size_t length) : m_data(data), m_length(length) {} | ||
| explicit MemoryStream(const uint8_t* data, std::size_t length) : m_data(data), m_length(length) |
There was a problem hiding this comment.
@JinmingHu-MSFT , is there a reason to have this memoryStream and the one from Azure::Core ?
I would say let's have just one
…ue-ptr-for-body-stream
…ue-ptr-for-body-stream
vhvb1989
requested review from
damirault,
danieljurek,
RickWinter and
a team
as code owners
|
/check-enforcer override |
| constexpr uint64_t fixedSize = 1024 * 15; | ||
| uint8_t tempBuffer[fixedSize]; |
There was a problem hiding this comment.
Both Azure Core SDK and Azure Storage SDK are supposed to be portable. So we cannot assume that on every platforms with all kinds of hardware configuration, the stack size is at least 16KB.
Changing stream body * to unique_ptr .
Storage unit tests passing.
I updated the protocol and convenient layer. I understand it is auto generated. But hopefully it can be used as reference of a working configuration.