You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "cstamas (via GitHub)" <gi...@apache.org> on 2023/02/24 15:02:59 UTC
[GitHub] [maven-resolver] cstamas opened a new pull request, #257: [MRESOLVER-329] More robust IO in DefaultTrackingFileManager
cstamas opened a new pull request, #257:
URL: https://github.com/apache/maven-resolver/pull/257
Make the code more robust to copy with concurrently accessed files and hopefully with Windows quirks re atomic move.
---
https://issues.apache.org/jira/browse/MRESOLVER-329
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #257: [MRESOLVER-329] More robust IO in DefaultTrackingFileManager
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #257:
URL: https://github.com/apache/maven-resolver/pull/257#discussion_r1117219091
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/FileUtils.java:
##########
@@ -105,7 +106,16 @@ public Path getPath() {
@Override
public void move() throws IOException {
- Files.move(tempFile, file, StandardCopyOption.ATOMIC_MOVE);
+ try {
+ Files.move(tempFile, file, StandardCopyOption.ATOMIC_MOVE);
Review Comment:
I might be wrong, but this is what Jenkins does as well: tries atomic, and if fails, the fallback is plain copy+rm... Naturally, this will need some triage, to see is it working at all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #257: [MRESOLVER-329] More robust IO in DefaultTrackingFileManager
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #257:
URL: https://github.com/apache/maven-resolver/pull/257#discussion_r1117219091
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/FileUtils.java:
##########
@@ -105,7 +106,16 @@ public Path getPath() {
@Override
public void move() throws IOException {
- Files.move(tempFile, file, StandardCopyOption.ATOMIC_MOVE);
+ try {
+ Files.move(tempFile, file, StandardCopyOption.ATOMIC_MOVE);
Review Comment:
I might be wrong, but this is what Jenkins does as well: tries atomic, and if fails, the fallback is plain copy+rm... Naturally, this will need some triage, to see is it working at all. Also, all this is for only one: Windows
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on pull request #257: [MRESOLVER-329] More robust IO in DefaultTrackingFileManager
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on PR #257:
URL: https://github.com/apache/maven-resolver/pull/257#issuecomment-1444066052
I'd like better understand whether the usecases file w/o temp files. I want to exclude our locks to be faulty here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] laeubi commented on a diff in pull request #257: [MRESOLVER-329] More robust IO in DefaultTrackingFileManager
Posted by "laeubi (via GitHub)" <gi...@apache.org>.
laeubi commented on code in PR #257:
URL: https://github.com/apache/maven-resolver/pull/257#discussion_r1117182543
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/FileUtils.java:
##########
@@ -105,7 +106,16 @@ public Path getPath() {
@Override
public void move() throws IOException {
- Files.move(tempFile, file, StandardCopyOption.ATOMIC_MOVE);
+ try {
+ Files.move(tempFile, file, StandardCopyOption.ATOMIC_MOVE);
Review Comment:
As mentioned in the other PR this is likely not what one wants. If the (atomic) move do not succeeds, there is no guarantee that the file has not changed (deleted, modified, ...) so retry the move will simply ignore this and overwrite everything. So callers of the move operation must retry the full operation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on a diff in pull request #257: [MRESOLVER-329] More robust IO in DefaultTrackingFileManager
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on code in PR #257:
URL: https://github.com/apache/maven-resolver/pull/257#discussion_r1117990047
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultTrackingFileManager.java:
##########
@@ -75,6 +76,8 @@ public Properties update(File file, Map<String, String> updates) {
if (Files.isReadable(filePath)) {
try (InputStream stream = Files.newInputStream(filePath)) {
props.load(stream);
+ } catch (NoSuchFileException e) {
+ // MRESOLVER-329: ignore; in case of concurrent r/w Files.isReadable may return true
Review Comment:
Please move this to another PR, both issues aren't necessarily related.
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/FileUtils.java:
##########
@@ -105,7 +106,16 @@ public Path getPath() {
@Override
public void move() throws IOException {
- Files.move(tempFile, file, StandardCopyOption.ATOMIC_MOVE);
+ try {
+ Files.move(tempFile, file, StandardCopyOption.ATOMIC_MOVE);
+ } catch (AccessDeniedException e) {
+ // MRESOLVER-329: fallback to plain copy+rm, this usually happens on Windows
Review Comment:
MRESOLVER-325 and not 329
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on pull request #257: [MRESOLVER-329] More robust IO in DefaultTrackingFileManager
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #257:
URL: https://github.com/apache/maven-resolver/pull/257#issuecomment-1446192026
Superseded by https://github.com/apache/maven-resolver/pull/260
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] michael-o commented on pull request #257: [MRESOLVER-329] More robust IO in DefaultTrackingFileManager
Posted by "michael-o (via GitHub)" <gi...@apache.org>.
michael-o commented on PR #257:
URL: https://github.com/apache/maven-resolver/pull/257#issuecomment-1445224171
Does not work and suffers from the chicken and egg problem described in here: https://issues.apache.org/jira/browse/MRESOLVER-325?focusedCommentId=17693545&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17693545
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas closed pull request #257: [MRESOLVER-329] More robust IO in DefaultTrackingFileManager
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas closed pull request #257: [MRESOLVER-329] More robust IO in DefaultTrackingFileManager
URL: https://github.com/apache/maven-resolver/pull/257
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org