smbj icon indicating copy to clipboard operation
smbj copied to clipboard

Unchecked exceptions

Open tliebeck opened this issue 7 years ago • 8 comments

I'd like to make the suggestion that unchecked exception use be eliminated within this project. My current strategy has been to wrap every call that interfaces with this library in a RuntimeException-catching try block, but this is far from an ideal solution.

I'm also seeing cases where RuntimeExceptions can be thrown in unexpected places, such as the FileInputStream.read() implementation. I'm having to create wrapper classes to deal with these. In my opinion anything going wrong in an InputStream's read() method should be dealt with using an IOException as the Java API specifies.

Also seeing background threads (which are managed entirely within the library) stop the JVM due to unchecked exceptions.

In my project, I absolutely cannot have an unhandled exception.

tliebeck avatar Aug 06 '17 08:08 tliebeck

Agreed, I will revisit the exception handling soon.

hierynomus avatar Aug 07 '17 08:08 hierynomus

@hierynomus What is your current progress on this issue? I was going to take a stab at it but don't want to duplicate any efforts you have already made.

NinjaChris16 avatar Aug 15 '17 18:08 NinjaChris16

I've made a start with it. The current idea:

  • Buffer.BufferException extends RuntimeException, as that should not be thrown unless we get malformed/malicious packets
  • SMBRuntimeException -> SMBException and no longer a RuntimeException
  • SMBApiException extends SMBException
  • async threads catch all exceptions and propagate them nicely (for instance the AsyncPacketReader

hierynomus avatar Aug 16 '17 11:08 hierynomus

I'd like to add on to this. There's at least one chicken-and-egg problem caused by what seems like excessive exception-throwing and I think it should be addressed. If DiskShare.folderExists() is called on a file or DiskShare.fileExists() is called on a folder, a runtime exception is thrown, so you need to know ahead of time whether you're looking at a file or a folder. However, you get that information from DiskShare.getFileInformation(), which also throws a runtime exception if you point it at a path where nothing exists. You can't find out if an object exists without knowing its type, and you can't find out its type without knowing it exists (at least, not without some ugly exception handling). The root cause of this interaction may be the way SMB was designed, but it would be nice if the API could abstract this limitation out somehow--perhaps add a public DiskShare.exists() method which is agnostic to the object type as far as the caller is concerned?

trdesilva avatar Nov 08 '17 21:11 trdesilva

The DiskShare.fileExists() and DiskShare.folderExists()` methods have been fixed in the current master already.

hierynomus avatar Nov 08 '17 21:11 hierynomus

Ah, I see #235 now; I was only looking at open issues before. Do you have an estimate for when that change will find its way into a release on Maven? Thanks.

trdesilva avatar Nov 08 '17 22:11 trdesilva

Release 0.5.1 has just been released to BinTray, so should be appearing in maven central soon.

hierynomus avatar Nov 09 '17 08:11 hierynomus

Do you still work on it ? Should I catch SMBApiException in my code ? I use AWS FSX. Sometimes the password expires and so my program throws an SMBApiException. How to properly deal with that situation ?

geniusit avatar Sep 08 '23 09:09 geniusit