New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't persist downloads to the cache until they complete #40

Merged
merged 2 commits into from Oct 5, 2017

Conversation

Projects
None yet
2 participants
@pimterry
Contributor

pimterry commented Oct 4, 2017

This is aiming to solve resin-io/resin-cli#541.

It doesn't completely solve #38, since we still can't easily check the integrity of our downloads, but it does stop us permanently caching downloads that don't really complete at all.

@pimterry pimterry requested review from jviotti and MoranF Oct 4, 2017

@@ -120,8 +120,15 @@ exports.getImageWritableStream = (deviceType, version) ->
# Ensure the cache directory exists, to prevent
# ENOENT errors when trying to write to it.
mkdirp(path.dirname(imagePath)).then ->
return fs.createWriteStream(imagePath)
# Append .inprogress to streams, move them to the right location only on success
inProgressPath = imagePath + '.inprogress'

This comment has been minimized.

@jviotti

jviotti Oct 5, 2017

Would it be good to delete this file if the stream emits an error event?

@jviotti

jviotti Oct 5, 2017

Would it be good to delete this file if the stream emits an error event?

This comment has been minimized.

@pimterry

pimterry Oct 5, 2017

Contributor

Great idea! Done, let me know what you think

@pimterry

pimterry Oct 5, 2017

Contributor

Great idea! Done, let me know what you think

@@ -36,6 +36,7 @@ doDownload = (deviceType, version) ->
cache.getImageWritableStream(deviceType, version)
.then (cacheStream) ->
pass.pipe(cacheStream)
pass.on('end', cacheStream.persistCache)

This comment has been minimized.

@jviotti

jviotti Oct 5, 2017

Should we wait until the cacheStream.persistCache promise is resolved before resolving this promise?

@jviotti

jviotti Oct 5, 2017

Should we wait until the cacheStream.persistCache promise is resolved before resolving this promise?

This comment has been minimized.

@pimterry

pimterry Oct 5, 2017

Contributor

That would be tidy, but we can't I think.

The promise resolves with the stream itself, and needs to resolve as soon as that starts, so we can start streaming it onwards, and show progress while the download runs. If we wait until the stream ends and the cache is persisted then we don't get that streaming goodness.

@pimterry

pimterry Oct 5, 2017

Contributor

That would be tidy, but we can't I think.

The promise resolves with the stream itself, and needs to resolve as soon as that starts, so we can start streaming it onwards, and show progress while the download runs. If we wait until the stream ends and the cache is persisted then we don't get that streaming goodness.

@jviotti

jviotti approved these changes Oct 5, 2017

@pimterry pimterry merged commit 53dc0f7 into master Oct 5, 2017

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
hound No violations found. Woof!

@pimterry pimterry deleted the dont-cache-failed branch Oct 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment