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

feat(#2375): fix the bug with AccessDenied exception in TranspileMojo #2380

Merged
merged 7 commits into from
Aug 14, 2023

Conversation

volodya-lombrozo
Copy link
Member

@volodya-lombrozo volodya-lombrozo commented Aug 11, 2023

Fix the problem with AccessDeniedException in TranspileMojo

Closes: #2375, #2370


PR-Codex overview

Detailed summary

  • Removed the import statement for java.nio.file.AccessDeniedException in TranspileMojo.java.
  • Added a synchronization block to prevent AccessDeniedException when deleting files on Windows OS in the cleanUpClasses method.
  • Added a TODO task to remove the catch block for AccessDeniedException in the cleanUpClasses method.
  • Added a TODO task to add concurrency tests for the cleanUpClasses method.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@volodya-lombrozo volodya-lombrozo marked this pull request as ready for review August 11, 2023 13:19
@volodya-lombrozo
Copy link
Member Author

@Graur Could you take a look, please?

Copy link
Contributor

@Graur Graur left a comment

Choose a reason for hiding this comment

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

@volodya-lombrozo Seems like a good solution, but there are a couple of questions:

* You can read more about the original problem in the following issue:
* - <a href="https://github.com/objectionary/eo/issues/2370">issue link</a>
* In other words, concurrent file deletions on the Windows OS can lead to an
* {@link java.nio.file.AccessDeniedException}, which could crash the build.
Copy link
Contributor

Choose a reason for hiding this comment

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

@volodya-lombrozo What do you think about creating a test case for such an issue? Maybe something like this https://www.yegor256.com/2018/03/27/how-to-test-thread-safety.html

Copy link
Member Author

Choose a reason for hiding this comment

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

@Graur I'm already spend lots of time for that PR. Can we implement it the next PR? I have added a puzzle for it.

this,
String.format("Can't delete file %s due to access denied", binary)
);
synchronized (TranspileMojo.class) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@Graur What do you suggest to decorate? We don't have objects here. Or maybe I am missing something?

@volodya-lombrozo
Copy link
Member Author

@Graur Could you have a look one more time, please?

@volodya-lombrozo
Copy link
Member Author

@Graur reminder

Copy link
Contributor

@Graur Graur left a comment

Choose a reason for hiding this comment

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

@volodya-lombrozo Thanks!
@yegor256 Please have a look

@volodya-lombrozo
Copy link
Member Author

@yegor256 Could you merge that changes, please?

@yegor256
Copy link
Member

@rultor merge

@rultor
Copy link
Contributor

rultor commented Aug 14, 2023

@rultor merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

@rultor rultor merged commit d3caa91 into objectionary:master Aug 14, 2023
11 checks passed
@rultor
Copy link
Contributor

rultor commented Aug 14, 2023

@rultor merge

@yegor256 Done! FYI, the full log is here (took me 12min)

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.

TranspileMojo.java:255-259: Remove AccessDeniedException...
4 participants