Error handling with retries in blockwise upload #489
Conversation
|
Visit the preview URL for this PR (updated for commit 929951b): https://golioth-firmware-sdk-doxygen-dev--pr489-rsv-handle-upl-g63df03m.web.app (expires Tue, 21 May 2024 04:12:54 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: a9993e61697a3983f3479e468bcb0b616f9a0578 |
rashmivaidya72
force-pushed
the
rsv/handle_upload_errors
branch
from
3d11196
to
f40b095
Compare
Code Coverage
|
Signed-off-by: Rashmi Vaidya <rashmi.vaidya72@gmail.com>
rashmivaidya72
force-pushed
the
rsv/handle_upload_errors
branch
from
f40b095
to
929951b
Compare
| { | ||
| // TODO: handle errors like disconnects etc | ||
| return GOLIOTH_OK; | ||
| while (1) |
There was a problem hiding this comment.
We should check (and decrement) the retry count here in the while(). I also don't think we need to keep retry_count in the context struct.
| @@ -134,10 +188,16 @@ enum golioth_status golioth_blockwise_post(struct golioth_client *client, | |||
| read_block_cb cb, | |||
| void *callback_arg) | |||
| { | |||
| enum golioth_status status = GOLIOTH_ERR_FAIL; | |||
| enum golioth_status status; | |||
There was a problem hiding this comment.
This can be declared closer to use.
| { | ||
| status = upload_single_block(client, ctx); | ||
| upload_single_block(client, ctx); | ||
| if (ctx->status == GOLIOTH_OK) |
There was a problem hiding this comment.
It feels confusing to be mixing storing the status in the context and keeping it local on the stack. I'm not sure that we need it in the context struct.
There was a problem hiding this comment.
I see. The status in the context struct is used to store the status that we get from the server response that is passed to us via a callback executed on the CoAP client thread. But because we are doing synchronous operations, the server response status is also returned to us directly, so we don't even actually need the callback.
On the other hand, the synchronous functions internal to the coap client create and destroy a semaphore for each transaction (i.e. for each block that we download). It would be more efficient to use asynchronous transfers and to make the operations blocking by waiting on a semaphore that is managed by the blockwise transfers module and created once per blockwise transfer.
| if (ctx->status == GOLIOTH_OK) | ||
| { | ||
| ctx->offset++; | ||
| } | ||
| else | ||
| { | ||
| // TODO: handle_upload_error | ||
| status = handle_upload_error(); | ||
| status = handle_upload_error(client, ctx); |
There was a problem hiding this comment.
If we fall into the else clause we never increment the offset.
No description provided.