Skip to content
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

#794 Node, npm and yarn installer delete corrupted archive #807

Merged
merged 1 commit into from
Apr 16, 2019

Conversation

seregamorph
Copy link
Contributor

@seregamorph seregamorph commented Apr 16, 2019

Summary
Issue #794
The node, npm and yarn installer deletes corrupted archive and destination directory in case if the downloading was interrupted (causes EOFException when extracted).

Detailed
The possible real-life example:

  • The build is started and at the phase of downloading npm, node or yarn it is interrupted on the slow connection, the build fails
  • Re-start the build - it has lazy download logic and determines that the destination archive is already in place
  • The archive is extracted and fails with root cause EOFException, the build fails
    • Re-start the build and either node (npm / yarn file) is here, but the package is incomplete - the build may fail
    • or the previous step (extract archive) is repeated again and again fails.
      This requires manual investigation and deleting the cached archive and can take much time.

How to simulate it locally.

  • Make a successful build of npm (yarn) project.
  • Go to $HOME/.m2/repository/com/github/eirslett/node/${node.version} directory
  • Rename the existing archive to -full suffix and make it's partial copy of first bytes:
dd if=node-8.9.4-darwin-x64.tar.gz-full of=node-8.9.9-darwin-x64.tag.gz bs=1000000 count=1
  • Remove project cached node version
  • Run the build
  • Re-run the build

Please note, that no integration tests are provided for the reasons of non-clear approach for it (the build should be interrupted while the archive is being downloaded; or it can be pre-cached manually to the maven directory). I tested it locally step-by-step.

Also please note, that there is another way to solve this:
The downloading should be done to a temp file unless completed and then renamed to destination. Both approaches can be mixed in case if someone will face the described above scenario expecting that it will be solved with plugin update.

this.logger.error("The archive file {} is corrupted and will be deleted. "
+ "Please try the build again.", archive.getPath());
archive.delete();
if (packageDirectory.exists()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the "package" directory is partially filled (dirty), for this reason should be purged

this.logger.error("The archive file {} is corrupted and will be deleted. "
+ "Please try the build again.", archive.getPath());
archive.delete();
FileUtils.deleteDirectory(tmpDirectory);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to check that tmpDirecory exists (guaranteed by the flow)

this.logger.error("The archive file {} is corrupted and will be deleted. "
+ "Please try the build again.", archive.getPath());
archive.delete();
if (installDirectory.exists()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above code cleanups the installDirectory, but there is a probabilty to use corrupted (incomplete) Yarn dist.

The node, npm and yarn installer deletes corrupted archive and destination directory in case if the downloading was interrupted (causes EOFException when extracted).
}
}

throw e;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the possible alternate option here is to re-download one more time
but the probability of this failure is minor, so in case if it happens the user should not even understand what happened :D

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related issue - #882 - auto-delete triggers but the auto-retry probably would be nice as a future enhancement

Copy link

@ryanrupp ryanrupp May 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, downloader would benefit from downloading to a tmp location and then moving post download, I put a comment here.

@eirslett
Copy link
Owner

Thanks, that looks good!

@eirslett eirslett merged commit 1fdf210 into eirslett:master Apr 16, 2019
@seregamorph seregamorph deleted the ISSUE-794 branch September 28, 2020 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants