Fix mender-update hanging when long-term network issues happen during Artifact download #1992
Conversation
|
@vpodzime, start a full client pipeline with:
my commands and optionsYou can prevent me from automatically starting CI pipelines:
You can trigger a client pipeline on multiple prs with:
You can trigger a client pipeline for a specific Mender Client release with:
You can trigger GitHub->GitLab branch sync with:
You can print PR statistics for a repository with:
You can deploy a review app with:
You can run e2e tests against a deployed review app with:
You can cherry pick to a given branch or branches with:
|
|
@lluiscampos I'd appreciate your 👀 on this as well if you have a minute. 🍻 |
…erAsyncReader The last_read_ struct records information about the last call of AsyncRead() which is a DownloadResumerAsyncReader's function. It makes more sense to record this information inside that class then. The strange `friend` declaration allowing a DownloadResumerAsyncReader instance contained inside DownloadResumerClient to access DownloadResumerClient's private members can now be dropped and the hierarchy makes more sense. Ticket: MEN-9954 Changelog: None Signed-off-by: Vratislav Podzimek <vratislav.podzimek+auto-signed@northern.tech>
50b488b to
d998f81
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
Hello 😺 I created a pipeline for you here: Pipeline-2681806525 Build Configuration Matrix
|
83ebfd5 to
77fcfae
Compare
…stream When there are network issues during Artifact download and the HTTP download resumer exhausts its retry backoff, the only way to deliver the error to the state machine is through the HTTP **body reader's** handler because the **body handler** passed to the DownloadResumerClient is only called when all body data is fetched which normally happens in a completely different state than the one that starts the download and thus the UpdateDownloadState doesn't handle any errors in it. Add a Fail() function to the body reader and use it to make sure it calls its handler with the respective error. Also add a comment about how the DownloadResumerClient is used. Ticket: MEN-9954 Changelog: Fixed mender-update hanging indefinitely (never reporting a deployment failure and never returning to the poll loop) when a network outage during artifact download lasted long enough for the download resumer to give up on resuming. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Vratislav Podzimek <vratislav.podzimek+auto-signed@northern.tech>
Unlike the basic http::Client which constructs the reader and returns it, leaving the API user code to manage the reader's lifetime, the DownloadResumerClient needs to keep a reference (shared pointer) to the reader too and thus manage its lifetime. The reason for this is that while the http::Client's reader (and its lifetime) is bound to a socket that gets closed on an error or completion, only the DownloadResumerClient itself knows when the body reader (providing data from multiple HTTP requests/responses) won't produce more data, i.e. when no more (re)tries will be performed. Tests that were exercising the reader ownership by the API user code were adjusted to test cancellation of the reader instead. Ticket: MEN-9954 Changelog: None Signed-off-by: Vratislav Podzimek <vratislav.podzimek+auto-signed@northern.tech>
…omAsyncReader::Read() The handler passed to the asynchronous reader can be called with a different stack (frame) than that of the ReaderFromAsyncReader::Read() function and then those references (pointers) lead to random unrelated places. We need to use shared pointers to make sure the referenced data is valid independently on the stack. Ticket: MEN-9954 Changelog: None Signed-off-by: Vratislav Podzimek <vratislav.podzimek+auto-signed@northern.tech>
f5144b4 to
63faa55
Compare
…:Read() The handler passed to the asynchronous reader can be called multiple times after adding a call to `AsyncRead()` to the event loop, not only once when there is data available, but also if the asynchronous reader encounters an error. Since the event loop is only run recursively **once** per ReaderFromAsyncReader::Read() call, we need to make sure it's also stopped only once. Otherwise it may stop the event loop for good. Ticket: MEN-9954 Changelog: None Signed-off-by: Vratislav Podzimek <vratislav.podzimek+auto-signed@northern.tech>
63faa55 to
d0a4eb0
Compare
|
Merging these commits will result in the following changelog entries: Changelogsmender (master-resumer-giveup-hangs-async-reader)New changes in mender since master: Bug Fixes
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@mender-test-bot start client pipeline |
|
Hello 😺 I created a pipeline for you here: Pipeline-2694097193 Build Configuration Matrix
|
Btw, these are 3 lines that used to miss coverage that our now 4 lines missing coverage with 1 extra line and 3 lines with minimal changes. |
| logger_.Warning("DownloadResumerClient destroyed while request is still active!"); | ||
| } | ||
| client_.Cancel(); | ||
| resumer_reader_.reset(); |
There was a problem hiding this comment.
Why do you reset a shared_ptr here?If no one is using it, it will be destroyed here automatically. And if someone else is using it, this reset will do nothing from their perspective, their shared_ptr will still point to resumer_reader_.
| DoCancel(); | ||
| user_body_handler_(exp_resp); | ||
| // we are done, the body reader won't produce any more data | ||
| resumer_reader_.reset(); |
There was a problem hiding this comment.
Why do we need all those resumer_reader_.reset() in multiple places? Isn't it called only when we already cancelled our DownloadResumerClient, meaning that it will not use this shared_ptr anymore and will be cleared on DownloadResumerClient destruction?
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
@mender-test-bot start client pipeline --pr integration/2951 |
|
Hello 😺 I created a pipeline for you here: Pipeline-2699405995 Build Configuration Matrix
|
No description provided.