[WIP] Use exceptions in SFTP backend

[RobinMcCorkell]

Exceptions are reported for errors instead of returning false. If an exception occurs during connection, a StorageNotAvailableException is thrown. One difference is that files that cannot be accessed will give an 'Unknown error' in the web UI, instead of allowing access to the folder but silently failing on anything inside it. It's not ideal, but better than before IMHO.

This commit also makes the file cache scanner more robust when it comes to exceptions - there are a few more try {} catch {} blocks in strategic locations.

Fixes #13238 (needs tested though!)

cc @icewind1991 @PVince81 @LukasReschke @DeepDiver1975 @MorrisJobke

[MorrisJobke]

It feels wrong if an Exception simply get catched and nothing is done. Isn't logging this reasonable?

[RobinMcCorkell]

@MorrisJobke the way I look at it, is that an exception occurs when something exceptional happens - for example if a function that is to get some property of a file fails to get that property. However, in some circumstances such errors should be ignored (for the greater good), like in cache scanning. An exception while trying to access a file indicates that the file is otherwise unavailable, so in bulk scanning jobs (like backgroundScan or when recursively scanning children in scanChildren) these exceptions should be ignored - some files aren't accessible (for whatever reason, permissions, system failure, etc) but the others are OK and should be scanned. Logging is done if something truely bad happens.

As for the catch blocks in the SFTP backend - they are to mimic the old behaviour of the backend, which was to ignore missing host keys or being unable to write them. I understand missing host keys, but personally I think a failure to write them should be an error-raising exception 🙊

[scrutinizer-notifier]

The inspection completed: 6 new issues, 2 updated code elements

[RobinMcCorkell]

@icewind1991 what do you think of this approach? Is there any better way to handle exceptions in the filesystem view layer?

[RobinMcCorkell]

As mentioned in #13791, given that the storage subsystem isn't ready to handle exceptions yet, this needs to be postponed until we have a proper exception handling mechanism in place.

[PVince81]

I'd say the minimum would be try and catch the "storage not available" cases and throw these.
As said before we still need to keep some of the expected return false cases.

[RobinMcCorkell]

@PVince81 @icewind1991 In what direction should we take this? The ultimate goal as I see it is to have exceptions as the error handling mechanism in the filesystem layer, but I understand that at the moment the filesystem doesn't handle exceptions very well. How does this relate to the other filesystem-related PRs, like #11091? Do we want to focus on getting exception-safety for oC 8.2?

[PVince81]

I'm not sure, there is already a lot of stuff for 8.2, but we can tentatively set this to 8.2.

[PVince81]

@Xenopathic currently the only exceptions properly caught in core are StorageNotAvailableException and StorageInvalidException. Maybe we should add more like the auth one you proposed ?

Regarding catching an exception without handler, please add a comment inside to explain why it is doing nothing.

I guess this PR will probably have to wait further.

[RobinMcCorkell]

@PVince81 I had a better idea (I think I discussed it on IRC once): allow a Storage to define exceptions that come from underlying libraries and how to map them to OC exceptions (e.g. SFTP exceptions => StorageNotAvailableException), which can then be handled properly in core.

But having the AuthenticationFailedException would be handy to have in core.

[PVince81]

That sounds like a plan, have a look at how we did it for DAV storages: https://github.com/owncloud/core/blob/master/lib/private/files/storage/dav.php#L787

Maybe this could be done in a more generic way.

[RobinMcCorkell]

@PVince81 Brilliant! A working implementation of that idea, perfect. For a more generic method, how about each class simply enumerates the exception map as an array, then a generic convertException() method simply iterates over that array and converts as necessary?

E.g.

public function getExceptionMap() {
    return [
        '\Some\Vendor\Library\Exception' => '\OCP\Files\StorageNotAvailableException',
    ];
}

// convertException()
foreach ($storage->getExceptionMap() as $vendorException => $ocException) {
    if ($e instanceof $vendorException) {
        throw new $ocException(params here);
    }
}

Don't know what the performance of that will be... then again, exceptions are exceptional events, not regular or frequent.

[PVince81]

That could work.
Or how about simply keeping the method new "convertException" and let it be implemented by the subclass however they wish ? This would keep room for special logic and parameter conversion, if needed.

[RobinMcCorkell]

@PVince81 Yeah, I think we should keep it simple and follow what the DAV storage does, a convertException() method will work just fine. I'll keep this PR around until I figure out what I'm doing.

[MorrisJobke]

Talked to @Xenopathic and we will move this to 9.0 because it would be nice to have this merged early in the development cycle.

[PVince81]

@Xenopathic any update?

[PVince81]

Needs rebase and probably defer to 9.1 ?

[MorrisJobke]

@Xenopathic Is this really needed anymore? I thought there went other fixes into the files_external. If there is no response within the next weeks I will close this, because the underlying code is restructured a lot. 😝

[PVince81]

@MorrisJobke unfortunately no. We still require every external storage backend to properly throw StorageNotAvailable when an obvious "no connection" error happens.

[PVince81]

and with "no", I mean "yes" to you question "Is this really needed anymore?"

[PVince81]

@Xenopathic can we move this forward ?

[PVince81]

@Xenopathic revive or close ? This PR is more than a year old.
I'd suggest closing and revisiting exception handling in the future.

[RobinMcCorkell]

Yeah, close. This would need to be mostly rewritten anyway

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.