Skip to content
This repository has been archived by the owner on Dec 7, 2019. It is now read-only.

readAll() is not resilient to exceptions reading a single file #293

Open
riclage opened this issue Nov 23, 2017 · 4 comments · May be fixed by #296
Open

readAll() is not resilient to exceptions reading a single file #293

riclage opened this issue Nov 23, 2017 · 4 comments · May be fixed by #296

Comments

@riclage
Copy link

riclage commented Nov 23, 2017

The current readAll() implementation in FSAllReader fails if there is an exception reading a single file in the path. In this code:

        bufferedSourceObservable = Observable
                    .fromIterable(fileSystem.list(path))
                    .map(s -> {
                           try {
                                return fileSystem.read(s);
                            } catch (FileNotFoundException e) {
                                throw Exceptions.propagate(e);
                            }
                    });

If an exception occurs on a fileSystem.read(s), then the whole chain will stop with an onError emission preventing us from getting subsequent files on the path.

One way around it, I think, would be to wrap the read in a flatMap() and emit an empty or special type empty buffer. Something like:

    @Nonnull
    @Override
    public Observable<BufferedSource> readAll(@Nonnull final String path) throws FileNotFoundException {
        return Observable.defer(() -> {
            Observable<BufferedSource> bufferedSourceObservable;
            try {
                bufferedSourceObservable = Observable.fromIterable(fileSystem.list(path))
                    .flatMap(s -> Observable.just(fileSystem.read(s))
                        .onErrorReturn(throwable -> Okio.buffer((Source) new ExceptionBuffer(throwable))));
            } catch (FileNotFoundException e) {
                throw Exceptions.propagate(e);
            }
            return bufferedSourceObservable;
        });
    }

Then we can let the caller decide what to do with the ExceptionBuffer. Would something in this direction work?

@digitalbuddha
Copy link
Contributor

Yup sounds like a great solution. Want to pr it in with some tests?

@riclage
Copy link
Author

riclage commented Nov 24, 2017

Cool, thanks. Yes, I will try to do one.

@riclage
Copy link
Author

riclage commented Dec 4, 2017

@digitalbuddha Did you have a chance to look at my PR?

@digitalbuddha
Copy link
Contributor

digitalbuddha commented Dec 4, 2017 via email

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants