Improve error logging in _sftp_channel

[wrvdklooster]

In the _sftp_channel function if for example an "Operation unsupported" occurs the error logging will be "Failed Directory Change".

This is due to the except IOError as err after opening the channel.

I propose to move the directory change error handling to the channel.chdir.

In case of an IOError also the channel is not closed. Not sure if this is by design.

If you want I can create PR.

[byteskeptical]

Howdy Richard,

PR's are always welcome just note the suggestions in the contributing doc. In this case I started a PR as an init point to gauge expectations and to start getting some feedback from anyone willing to test. channel.chdir is a Paramiko call making it not an option for re-location unfortunately. After a bit of back and forth on my part I think at least for now, I want to try to keep it all housed inside _sftp_channel. If you'd like to submit an alternative suggestion or implementation feel free to do so.

[wrvdklooster]

I think I did not explain the issue well enough maybe.

Checking the PR you started seems a lot more is changed than I would expect.

As an example I just changed the error handling below.

Issue in my case was I had a mock during tests for sftp which dit not implement the posix_rename. I got the exception "Failed Directory Change" which was misleading. If I make the change below I get "Unsupported Operation" which makes more sense.

try:
    meta.settimeout(self._timeout)
    self._cache.__dict__.setdefault('cwd', self._default_path)

    if self._cache.cwd:
        try:
            channel.chdir(drivedrop(self._cache.cwd))
        # Separate error handling for the chdir
        except SFTPError as err:
            log.error(f'Failed Directory Change: [{self._cache.cwd}]')
            raise err
    else:
        self._cache.cwd = '/'
    log.info(f'Current Working Directory: [{self._cache.cwd}]')

    yield channel
# Error handling for opening channel
except Exception as err:
    if channel:
        channel.close()
    raise err
finally:
    if not meta.closed:
        self._channels[channel_name]['busy'] = False

[byteskeptical]

Thanks for the clarification. I think we are on the same page. The issue your being affected by is a subset of a larger issue with the same symptom. Namely than error's are being obfuscated by other messages (in the IOError situation like yours) or just passed to the catch-call Exception and raised whether or not it was a fatal error. While I like the keep it simple approach to your code I don't want to fix this for this one case only. For example even with moving the SFTPError within it's own try/catch and raising the err you lose the ability to recover from a non-fatal operation. Having the built-in retry decorator and trying to handle thread reuse effeciently leads me to the solution proposed. Trying to balance useful messaging to the user while not forcing failure if not necessary.