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.

Sign up for GitHub

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

Jump to bottom

Replaced own walkFileTree with correct implementation from nnio package. #12964

Merged
merged 4 commits into from May 21, 2024
Merged

Replaced own walkFileTree with correct implementation from nnio package. #12964

merged 4 commits into from May 21, 2024

Conversation

Bensikrac
Copy link
Contributor

@Bensikrac Bensikrac commented May 2, 2024 •
edited

This is another fix for that nullpointer issue as in #12476 (#12467)
Apparently there was a custom implementation of the same walkFileTree-function inside of the code, suprise still had the bug.
The new function, as per nnio lib is Files.java which contains the null checks:

public static Path walkFileTree(Path start, FileVisitor<? super Path> visitor) throws IOException {
    File file = start.toFile();
    if (!file.canRead()) {
      visitor.visitFileFailed(start, new AccessDeniedException(file.toString()));
      return start;
    }
    if (Files.isDirectory(start)) {
      FileVisitResult preVisitDirectoryResult = visitor.preVisitDirectory(start, null);
      if (preVisitDirectoryResult == FileVisitResult.CONTINUE) {
        File[] children = start.toFile().listFiles();
        if (children != null) {
          for (File child : children) {
            walkFileTree(FileBasedPathImpl.get(child), visitor);
          }
          visitor.postVisitDirectory(start, null);
        }
      }
      // Only FileVisitResult.CONTINUE and FileVisitResult.SKIP_SUBTREE are implemented.
      // FileVisitResult.SKIP_SUBTREE is handled simply by the fact that nothing is done for it.
    } else {
      visitor.visitFile(start, new BasicFileAttributes(file));
    }
    return start;
  }

(Edit: replaced with actual new source code from 0.3.1)
instead of the one in the file.

Functionally nothing should change, since visitFileFailed gets modified, that it continues no matter what, which is the only real change in the function.

This also somehow slipped by, because the dev build 20240209 worked only with the nnio version update for some reason.
Got reason:
#12323 added back the broken code without the null check (caused by nnio lib 0.3 not being spec by ignoring not getting FileVisitResult.CONTINUE and therefore needing to modify the loop function (this is fixed in nnio 0.3.1)
Here are the changes in nnio 0.3.1: #5

Also here is a stack trace of the issue:

05-02 01:12:25.531 25980 26045 D FileSyncHelper: File-sync start check folder /storage/emulated/0
05-02 01:12:25.540 25980 25980 I WM-SystemFgDispatcher: Started foreground service Intent { act=ACTION_START_FOREGROUND cmp=com.nextcloud.client/androidx.work.impl.foreground.SystemForegroundService (has extras) }
05-02 01:12:25.547 25980 26000 E WM-WorkerWrapper: Work [ id=b0e6b726-2807-4ed6-9fdb-920bba5be2a9, tags={ com.nextcloud.client.jobs.FilesSyncWork, *, name:periodic_files_sync, timestamp:1714604245289, class:FilesSyncWork } ] failed because it threw an exception/error
05-02 01:12:25.547 25980 26000 E WM-WorkerWrapper: java.util.concurrent.ExecutionException: java.lang.NullPointerException: Attempt to get length of null array
05-02 01:12:25.547 25980 26000 E WM-WorkerWrapper:      at androidx.work.impl.utils.futures.AbstractFuture.getDoneValue(AbstractFuture.java:515)
05-02 01:12:25.547 25980 26000 E WM-WorkerWrapper:      at androidx.work.impl.utils.futures.AbstractFuture.get(AbstractFuture.java:474)
05-02 01:12:25.547 25980 26000 E WM-WorkerWrapper:      at androidx.work.impl.WorkerWrapper$2.run(WorkerWrapper.java:316)
05-02 01:12:25.547 25980 26000 E WM-WorkerWrapper:      at androidx.work.impl.utils.SerialExecutorImpl$Task.run(SerialExecutorImpl.java:96)
05-02 01:12:25.547 25980 26000 E WM-WorkerWrapper:      at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
05-02 01:12:25.547 25980 26000 E WM-WorkerWrapper:      at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:644)
05-02 01:12:25.547 25980 26000 E WM-WorkerWrapper:      at java.lang.Thread.run(Thread.java:1012)
05-02 01:12:25.547 25980 26000 E WM-WorkerWrapper: Caused by: java.lang.NullPointerException: Attempt to get length of null array
05-02 01:12:25.547 25980 26000 E WM-WorkerWrapper:      at com.owncloud.android.utils.FileUtil.walkFileTree(FileUtil.java:76)
05-02 01:12:25.547 25980 26000 E WM-WorkerWrapper:      at com.owncloud.android.utils.FileUtil.walkFileTree(FileUtil.java:77)
05-02 01:12:25.547 25980 26000 E WM-WorkerWrapper:      at com.owncloud.android.utils.FileUtil.walkFileTree(FileUtil.java:77)
05-02 01:12:25.547 25980 26000 E WM-WorkerWrapper:      at com.owncloud.android.utils.FilesSyncHelper.insertCustomFolderIntoDB(FilesSyncHelper.java:66)
05-02 01:12:25.547 25980 26000 E WM-WorkerWrapper:      at com.owncloud.android.utils.FilesSyncHelper.insertAllDBEntriesForSyncedFolder(FilesSyncHelper.java:141)
05-02 01:12:25.547 25980 26000 E WM-WorkerWrapper:      at com.owncloud.android.utils.FilesSyncHelper.insertAllDBEntries(FilesSyncHelper.java:151)
05-02 01:12:25.547 25980 26000 E WM-WorkerWrapper:      at com.nextcloud.client.jobs.FilesSyncWork.collectChangedFiles(FilesSyncWork.kt:160)
05-02 01:12:25.547 25980 26000 E WM-WorkerWrapper:      at com.nextcloud.client.jobs.FilesSyncWork.doWork(FilesSyncWork.kt:118)
05-02 01:12:25.547 25980 26000 E WM-WorkerWrapper:      at androidx.work.Worker$1.run(Worker.java:82)
05-02 01:12:25.547 25980 26000 E WM-WorkerWrapper:      ... 3 more
05-02 01:12:25.548 25980 26000 I WM-WorkerWrapper: Worker result FAILURE for Work [ id=b0e6b726-2807-4ed6-9fdb-920bba5be2a9, tags={ com.nextcloud.client.jobs.FilesSyncWork, *, name:periodic_files_sync, timestamp:1714604245289, class:FilesSyncWork } ]
  • Tests written, or not not needed

@Bensikrac Bensikrac marked this pull request as ready for review May 10, 2024 12:12
@Bensikrac
Copy link
Contributor Author

@AndyScherzinger could you approve the workflow?
Also the PR is now final, I patched nnio and added the updated version (0.3.1)

@github-actions GitHub Actions
Copy link

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

@Bensikrac @AndyScherzinger
Replaced own walkFileTree with correct implementation from nnio package.
801e00c 
This prevents a NullPointerException on the case, that listFiles() returns a null pointer,
as that is allowed per spec.

Signed-off-by: Benedek Major <benedek@major.onl>
@Bensikrac @AndyScherzinger
Bump org.lukhnos.nnio from 0.3 to 0.3.1
1cdb0c7 
Signed-off-by: Benedek Major <benedek@major.onl>
@Bensikrac @AndyScherzinger
Updated verification metadata for nnio 0.3.1
056e4ae 
Signed-off-by: Benedek Major <benedek@major.onl>
@Bensikrac @AndyScherzinger
Replace null value with new SKIP_SUBTREE enum option
a04edc5 
Signed-off-by: Benedek Major <benedek@major.onl>
@AndyScherzinger AndyScherzinger merged commit 4be4841 into nextcloud:master May 21, 2024
19 of 20 checks passed
@AndyScherzinger AndyScherzinger added this to the Nextcloud App 3.30.0 milestone May 21, 2024
@AndyScherzinger
Copy link
Member

Nice one @Bensikrac and thanks for the fix/update/dependency-bump

I have been traveling last week, but rebased/merged it now 🎉

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

@AndyScherzinger AndyScherzinger AndyScherzinger approved these changes

Assignees
No one assigned
Labels
feedback-requested
Projects
None yet
Milestone
Nextcloud App 3.30.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Bensikrac @AndyScherzinger