Skip to content
Permalink
Browse files

Improve zero-length content handling in HttpClient object.

If content was zero-length then the IO object was not created.  This put the burden on the caller to test that the IO object existed before checking eof.

Instead, create an IO object even if it will immediately return eof.  This has little cost and makes the calling code simpler.

Also add an explicit test for zero-length files in S3 and a few assertions.
  • Loading branch information...
dwsteele committed May 9, 2019
1 parent 1553115 commit d5fac35fe3efe79f2b39a0e1b5ad3bb1cb1dd173
@@ -55,6 +55,10 @@
<p>Add <code>pathExists()</code> to <code>Storage</code> object.</p>
</release-item>

<release-item>
<p>Improve zero-length content handling in <code>HttpClient</code> object.</p>
</release-item>

<release-item>
<p>Don't append <code>strerror()</code> to error message when <code>errno</code> is 0.</p>
</release-item>
@@ -372,14 +372,15 @@ httpClientRequest(
HTTP_HEADER_CONTENT_LENGTH);
}

// If content chunked or content length > 0 then there is content to read
if (this->contentChunked || this->contentSize > 0)
{
this->contentEof = false;
// Was content returned in the response?
bool contentExists = this->contentChunked || this->contentSize > 0;
this->contentEof = !contentExists;

// If all content should be returned from this function then read the buffer. Also read the reponse if there
// has been an error.
if (returnContent || httpClientResponseCode(this) != 200)
// If all content should be returned from this function then read the buffer. Also read the reponse if there has
// been an error.
if (returnContent || httpClientResponseCode(this) != HTTP_RESPONSE_CODE_OK)
{
if (contentExists)
{
result = bufNew(0);

@@ -390,19 +391,21 @@ httpClientRequest(
}
while (!httpClientEof(this));
}
// Else create the read interface
else
}
// Else create an io object, even if there is no content. This makes the logic for readers easier -- they can just
// check eof rather than also checking if the io object exists.
else
{
MEM_CONTEXT_BEGIN(this->memContext)
{
MEM_CONTEXT_BEGIN(this->memContext)
{
this->ioRead = ioReadNewP(this, .eof = httpClientEof, .read = httpClientRead);
ioReadOpen(this->ioRead);
}
MEM_CONTEXT_END();
this->ioRead = ioReadNewP(this, .eof = httpClientEof, .read = httpClientRead);
ioReadOpen(this->ioRead);
}
MEM_CONTEXT_END();
}
// If the server notified that it would close the connection after sending content then close the client side
else if (this->closeOnContentEof)

// If the server notified that it would close the connection and there is no content then close the client side
if (this->closeOnContentEof && !contentExists)
tlsClientClose(this->tls);

// Retry when reponse code is 5xx. These errors generally represent a server error for a request that looks valid.
@@ -22,7 +22,7 @@ typedef struct StorageReadS3
{
MemContext *memContext; // Object mem context
StorageReadInterface interface; // Interface
StorageS3 *storage; // Storage that created this object
StorageS3 *storage; // Storage that created this object

HttpClient *httpClient; // Http client for requests
} StorageReadS3;
@@ -83,6 +83,7 @@ storageReadS3(THIS_VOID, Buffer *buffer, bool block)
FUNCTION_LOG_END();

ASSERT(this != NULL && this->httpClient != NULL);
ASSERT(httpClientIoRead(this->httpClient) != NULL);
ASSERT(buffer != NULL && !bufFull(buffer));

FUNCTION_LOG_RETURN(SIZE, ioRead(httpClientIoRead(this->httpClient), buffer));
@@ -101,6 +102,7 @@ storageReadS3Eof(THIS_VOID)
FUNCTION_TEST_END();

ASSERT(this != NULL && this->httpClient != NULL);
ASSERT(httpClientIoRead(this->httpClient) != NULL);

FUNCTION_TEST_RETURN(ioReadEof(httpClientIoRead(this->httpClient)));
}
@@ -444,6 +444,7 @@ testRun(void)
httpClientRequest(client, strNew("GET"), strNew("/"), query, headerRequest, NULL, false), "request with no content");
TEST_RESULT_UINT(httpClientResponseCode(client), 200, " check response code");
TEST_RESULT_STR(strPtr(httpClientResponseMessage(client)), "OK", " check response message");
TEST_RESULT_UINT(httpClientEof(client), true, " io is eof");
TEST_RESULT_STR(
strPtr(httpHeaderToLog(httpClientReponseHeader(client))), "{connection: 'ack', key1: '0', key2: 'value2'}",
" check response headers");
@@ -100,6 +100,10 @@ testS3Server(void)
harnessTlsServerExpect(testS3ServerRequest(HTTP_VERB_GET, "/file.txt", NULL));
harnessTlsServerReply(testS3ServerResponse(200, "OK", NULL, "this is a sample file"));

// Get zero-length file
harnessTlsServerExpect(testS3ServerRequest(HTTP_VERB_GET, "/file0.txt", NULL));
harnessTlsServerReply(testS3ServerResponse(200, "OK", NULL, NULL));

// Throw non-404 error
harnessTlsServerExpect(testS3ServerRequest(HTTP_VERB_GET, "/file.txt", NULL));
harnessTlsServerReply(testS3ServerResponse(303, "Some bad status", NULL, "CONTENT"));
@@ -529,7 +533,7 @@ testRun(void)
}

// *****************************************************************************************************************************
if (testBegin("storageS3*(), StorageS3FileRead, and StorageWriteS3"))
if (testBegin("storageS3*(), StorageReadS3, and StorageWriteS3"))
{
testS3Server();

@@ -551,6 +555,7 @@ testRun(void)
TEST_RESULT_STR(
strPtr(strNewBuf(storageGetNP(storageNewReadNP(s3, strNew("file.txt"))))), "this is a sample file",
"get file");
TEST_RESULT_STR(strPtr(strNewBuf(storageGetNP(storageNewReadNP(s3, strNew("file0.txt"))))), "", "get zero-length file");

StorageRead *read = NULL;
TEST_ASSIGN(read, storageNewReadP(s3, strNew("file.txt"), .ignoreMissing = true), "new read file");

0 comments on commit d5fac35

Please sign in to comment.
You can’t perform that action at this time.