From caa377943a809ceffd40eef04a9f21a883e7f9cc Mon Sep 17 00:00:00 2001 From: Michael Scott Date: Tue, 22 Aug 2017 16:42:22 -0700 Subject: [PATCH] net: http: fix avoiding timeout on HTTP requests w/o body The original commit 8ebaf29927c2 ("net: http: dont timeout on HTTP requests w/o body") was intended to handle a case where an HTTP response had been retrieved from the server but the HTTP parser couldn't meet the criteria for calling "on_message_complete". For example, a POST to a REST API where the server doesn't return anything but an HTTP status code. It was a really bad idea to check a semaphore count. There is a lot of kernel logic built into semaphores and how the count is adjusted. The assumption that the value is 0 after the k_sem_give() is incorrect. It's STILL 0 if something is pending with a k_sem_take(). By the time k_sem_give() is done executing the other thread has now been kicked and the count is back to 0. This caused the original check to always pass and in turn breakage was noticed in the http_client sample. Let's do this the right way by setting a flag when on_message_complete is called and if that flag is not set by the time we reach recv_cb, let's give back the semaphore to avoid a timeout. Jira: ZEP-2561 Signed-off-by: Michael Scott --- include/net/http.h | 1 + subsys/net/lib/http/http_client.c | 9 ++++++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/include/net/http.h b/include/net/http.h index b466c0ede50..6a7ea296036 100644 --- a/include/net/http.h +++ b/include/net/http.h @@ -324,6 +324,7 @@ struct http_client_ctx { u8_t cl_present:1; u8_t body_found:1; + u8_t message_complete:1; } rsp; #if defined(CONFIG_HTTPS) diff --git a/subsys/net/lib/http/http_client.c b/subsys/net/lib/http/http_client.c index 7be65047f38..e8c800eaeda 100644 --- a/subsys/net/lib/http/http_client.c +++ b/subsys/net/lib/http/http_client.c @@ -367,6 +367,7 @@ static int on_message_complete(struct http_parser *parser) ctx->req.user_data); } + ctx->rsp.message_complete = 1; k_sem_give(&ctx->req.wait); return 0; @@ -464,6 +465,7 @@ int client_reset(struct http_client_ctx *ctx) ctx->rsp.content_length = 0; ctx->rsp.processed = 0; ctx->rsp.body_found = 0; + ctx->rsp.message_complete = 0; ctx->rsp.body_start = NULL; memset(ctx->rsp.response_buf, 0, ctx->rsp.response_buf_len); @@ -495,14 +497,15 @@ static void recv_cb(struct net_context *net_ctx, struct net_pkt *pkt, /* * This block most likely handles a TCP_FIN message. * (this means the connection is now closed) - * If we get here, and req.wait.count is still 0 this means - * http client is still waiting to parse a response body. + * If we get here, and rsp.message_complete is still 0 + * this means the HTTP client is still waiting to parse a + * response body. * This will will never happen now. Instead of generating * an ETIMEDOUT error in the future, let's unlock the * req.wait semaphore and let the app deal with whatever * data was parsed in the header (IE: http status, etc). */ - if (ctx->req.wait.count == 0) { + if (ctx->rsp.message_complete == 0) { k_sem_give(&ctx->req.wait); }