Enquiry about exception propagation in `transport.py`

[henrygoodman]

https://github.com/jborean93/smbprotocol/blob/master/src/smbprotocol/transport.py

In the smbprotocol.transport.Tcp.connect function, a ValueError is raised if the Tcp connection times out to the server.

This seems to propagate all the way up to the top through smbclient._pool.register_session, therefore if a server timeout occurs when registering the session, the client has to handle a ValueError.

Is there a specific reason this shouldn't be a custom exception (like a subclass of SMBException) so that the client doesn't catch more than they handle with this generic exception?

(Apologies if this is a stupid question, I am just running into the use case as per above so determining best approach)

[adiroiban]

Hi Henry,

Thanks for the report.

I am just answering as an user. I am not the mantainer of this project.

I guess you are refering to this code

https://github.com/jborean93/smbprotocol/blob/459100718eb6f54e6b7f90df6bb94f1a139e9b33/src/smbprotocol/transport.py#L65-L68

I think that the design of the current implementation is that any connection error (including a timeout) is converted into a ValueError.

For me it's ok not to have a SMBException. This is not realy a SMB protocol issue. This is a low-level TCP error.

Now, ValueError might not be the best exception for a TCP connection error, but this is what we have now.

The same ValueError is now raise on DNS resolving errors or TCP connection denied error.

---

For backward compatibility, I think is best to keep the current ValueError

---

It would be nice to have "native" errors, but this is what we have now.

Jordan was working on an async TCP layer rewrite. So maybe as part of that work, there is the option to redesign the TCP error handling.

But again, for backward compatibiliyt, we are kind of stuck with ValueError.

The code is here.

https://github.com/jborean93/smbprotocol/compare/socket-refactor

[jborean93]

Looks like it was originally added by https://github.com/jborean93/smbprotocol/commit/3b5bd3848efb99a4f33b02c40ad296ae1ca3ea3c, I think at the time I used ValueError to catch invalid addresses and just assumed it meant an invalid address was specified hence value error. Unfortunately that turned out to be the wrong choice and we are somewhat stuck with this behaviour for backwards compatibility. So yea as mentioned by @adiroiban not everything is an SMBException by default but this particular error should have probably stays an OSError.