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

fix(kernel): error raised during rename operation on win32 #1702

Merged
merged 2 commits into from
May 26, 2020

Conversation

RomainMuller
Copy link
Contributor

When loading a new library into the jsii kernel, the provided tarball
was extracted to a temporary directory, then moved into it's final
location. On Windows, this operation could fail on an EACCESS or
EPERM error (often due to malware scanners accessing the file for
inspection on file systems which do not support renaming files that are
being accessed).

This changes how the load API works so taht the tarball is extracted
directly in it's final install location, so that no rename operation is
needed.

Fixes #992


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

When loading a new library into the jsii kernel, the provided `tarball`
was extracted to a temporary directory, then moved into it's final
location. On Windows, this operation could fail on an `EACCESS` or
`EPERM` error (often due to malware scanners accessing the file for
inspection on file systems which do not support renaming files that are
being accessed).

This changes how the `load` API works so taht the `tarball` is extracted
directly in it's final install location, so that no rename operation is
needed.
@RomainMuller RomainMuller added bug This issue is a bug. effort/small Small work item – less than a day of effort contribution/core This is a PR that came from AWS. labels May 25, 2020
@RomainMuller RomainMuller requested a review from a team May 25, 2020 14:06
@RomainMuller RomainMuller self-assigned this May 25, 2020
@RomainMuller
Copy link
Contributor Author

Note: the change appears much bigger than it really is because it involves changing the indentation of a large chunk of code (no longer wrapped under a try { ... } finally { ... } block). You'll get a much better view on the actual change if you hide whitespace changes.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-Blkkw9bQFn8A
  • Commit ID: 000b3cd
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@rix0rrr
Copy link
Contributor

rix0rrr commented May 26, 2020

Guess I'm not necessarily opposed to this, but generally we do moves for a reason: to preserve atomicity.

Can you elaborate why this would not be an issue in this case? Why did we do the move in the first place?

@rix0rrr
Copy link
Contributor

rix0rrr commented May 26, 2020

Okay talked to Elad, it's not for atomicity reasons. Kay then.

@RomainMuller
Copy link
Contributor Author

RomainMuller commented May 26, 2020

Guess I'm not necessarily opposed to this, but generally we do moves for a reason: to preserve atomicity.

Can you elaborate why this would not be an issue in this case? Why did we do the move in the first place?

So the move was quite probably due to the fact the contents of the tarball is nested under an undesirable package directory, while we need to install into node_modules/<package-name> instead. Either the strip feature was unavailable (or undocumented) at the time we first wrote this, or it didn't occur to us we could use it.

But yeah it's not for atomicity since we should be able to assume only ONE thread is running through there at a time (node being collaborative multitasking only)

@mergify
Copy link
Contributor

mergify bot commented May 26, 2020

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label May 26, 2020
@mergify
Copy link
Contributor

mergify bot commented May 26, 2020

Merging (with squash)...

@rix0rrr
Copy link
Contributor

rix0rrr commented May 26, 2020

assume

Just going to leave this right here 😛

But yeah, let's try it, probably worth the risk.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-Blkkw9bQFn8A
  • Commit ID: 6fa2b8f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 38ee336 into master May 26, 2020
@mergify mergify bot deleted the rmuller/work-around-992 branch May 26, 2020 09:49
@mergify
Copy link
Contributor

mergify bot commented May 26, 2020

Merging (with squash)...

@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label May 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. contribution/core This is a PR that came from AWS. effort/small Small work item – less than a day of effort
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename operation throwing exception
3 participants