Cleanup download in all cases, avoid overwriting existing files #36
Conversation
| download oneShotDownload(uri, downloadFilePath); | ||
| download_cleanup_call scopedCleanup{&oneShotDownload}; | ||
|
|
||
| oneShotDownload.start(); |
There was a problem hiding this comment.
I noticed that a failure in start() was causing abort() on line 133 to not get called (exception gets thrown), leaving a stale download object in the agent.
shishirb-MSFT
force-pushed
the
shishirb/fix-download-cleanup
branch
from
d267b62 to
48dda26
Compare
| // Ensures download is finalized/aborted in the agent to avoid stale download objects. | ||
| // Future: We could expect callers to explicitly opt-in to persist downloads in the agent and | ||
| // use that as an indicator to perform/skip the cleanup directly in ~download(). | ||
| class download_cleanup_call |
There was a problem hiding this comment.
Does adding this cleanup impact the sdk when interacting with windows dosvc client? If so, consider adding this logic to the specific download_impl class #Closed
There was a problem hiding this comment.
This is required behavior for both windows and xplat.
| // error downloading to an already existing file. | ||
| if (_fDestFileCreated && !_destFilePath.empty()) | ||
| { | ||
| boost::filesystem::remove(_destFilePath); |
There was a problem hiding this comment.
boost::filesystem::remove(_destFilePath) [](http://example.com/codeflow?start=0&length=48)
Unrelated to this change but part of cleaning up our dependencies, the change you introduced already helps us remove usage of boost::filesystem, so there's potential to see if we can just remove some of the boost libraries in enterity #Closed
There was a problem hiding this comment.
boost::filesystem is still used in other parts like the rest_port_advetiser and finder. The biggest boost library is boost.log so I'm thinking of looking into that after the cpprestsdk work item.
|
|
||
| // Ensures download is finalized/aborted in the agent to avoid stale download objects. | ||
| // Future: We could expect callers to explicitly opt-in to persist downloads in the agent and | ||
| // use that as an indicator to perform/skip the cleanup directly in ~download(). |
There was a problem hiding this comment.
Does the agent currently have a self-cleanup mechanism to abort/expire jobs after a set period of time (24hr?) #Closed
There was a problem hiding this comment.
Windows dosvc does but the expiration is 30 days. ADU DO keeps it until shutdown (no persistence).
There was a problem hiding this comment.
And it's stored in memory until then? This could explain the memory usage I saw earlier (where after many downloads, our memory usage increased) - though this would only occur in the case the start threw. so maybe not
| ASSERT_TRUE(boost::filesystem::exists(g_tmpFileName)); // not deleted, the earlier download is still active | ||
|
|
||
| simpleDownload.abort(); | ||
| ASSERT_FALSE(boost::filesystem::exists(g_tmpFileName)); |
There was a problem hiding this comment.
what happens in the following case:
download 1 created
download 2 created
download 1 started
download 1 paused
download 1 abort
download 2 start
download 2 should expectedly succeed in this case correct? #Closed
There was a problem hiding this comment.
yes, I'll update the the test with this.
* Use CredScanSuppressions.json instead of inline comments (#15) * Add support for additional build types (#28) Add support for additional build types * Fix dockerfile for Deb10 (#29) * Support Ubuntu 20.04 arm64 builds and fix boost log linker issue (#31) * Update readme with section detailing install (#33) * Return specific errors from creating and accessing the destination file path (#35) * Provide better errors for dest file path. Use POSIX APIs. * Fix tests to account for this. * deliveryoptimization_set_iot_connection_string returns specific error codes * Fix WIL link * Cleanup download in all cases, avoid overwriting existing files (#36) * SDK: Ensure download is cleaned up in all cases * Agent: Avoid overwriting existing destination file * Agent: Fix file path logging in cancel event * Check binary size growth in azure pipelines (#38) * Check runs only for minsizerel flavor. * Baseline number obtained from local dev build. * Binary size increase of 5% or more results in build failure. Think carefully before increasing the baseline at any time. * SDK: Prepare to remove cpprestsdk dependency (#39) * CPortFinder provides port only, address constructed by CHttpClient. * Remove unused CHttpClient SendRequest method that supported request body. * Simplify SendRequest. Use boost ptree. * SDK: Remove cpprestsdk, use custom code with boost.asio (#40) * Custom, minimal, http parser implementation, on top of Boost.Asio. * Fix minsizerel build warning in class download. * Re-enable some tests. * Strip unneeded symbols from minsizerel binaries (#41) * Bootstrap only required boost libs * Strip unneeded symbols from minsizerel binaries. * Fix deb10 build. boost::asio::io_service backcompat. * Update limits for binary size check * Agent: Move log location to /var/log (#43) * This removes usage of /var/cache * Group write permissions needed only for the config path * Make strip_symbols optional (#44) * Some build environments do not have objcopy installed (like Yocto Zeus) * Agent: Update MCC fallback logic (#45) * Custom fallback logic: oscillate between MCC and original host when possible. * Ban MCC on fatal errors only. * 4xx errors are fatal for MCC also. * ConfigManager handling -1 fallback config. * Fix mcc manager tests not appending json files properly. * Agent: Reset retry delay on fallback from MCC * SDK: Add test to exercise new MCC fallback logic * Increment versions for release v0.7.0 (#50) agent, sdk: 0.5.0 plugin-apt: 0.3.0 * Fix github_release.yml (#52) * echo in Release Information task needs the text wrapped in quotes. * Use minsizerel for building all components. * Migrate azdo resources to 1ES managed pool (#51) * use ninja (#53) * Agent: Replace boost.log usage with custom logger (#54) * Agent: Remove boost.log requirement from test (#55) * SDK: Remove inline spaces in favor of nested namespaces (#57) Co-authored-by: shishirb-MSFT <50385517+shishirb-MSFT@users.noreply.github.com> Co-authored-by: Shishir Bhat <shishirb@microsoft.com>
No description provided.