Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add exception handling for SMBFileSystem init connection errors #1650

Merged
merged 6 commits into from
Jul 24, 2024
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 33 additions & 4 deletions fsspec/implementations/smb.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@
"""

import datetime
import re
import uuid
from stat import S_ISDIR, S_ISLNK

import smbclient
import smbprotocol.exceptions

from .. import AbstractFileSystem
from ..utils import infer_storage_options
Expand Down Expand Up @@ -127,8 +129,9 @@ def _port(self):

def _connect(self):
import time
retried_errors = []

for _ in range(self.register_session_retries):
for retry in range(self.register_session_retries):
try:
smbclient.register_session(
self.host,
Expand All @@ -138,9 +141,35 @@ def _connect(self):
encrypt=self.encrypt,
connection_timeout=self.timeout,
)
break
except Exception:
time.sleep(0.1)
return
except (
smbprotocol.exceptions.SMBAuthenticationError,
smbprotocol.exceptions.LogonFailure
):
# These exceptions should not be repeated, as they clearly indicate
# that the credentials are invalid and not a network issue.
raise
except ValueError as exc:
if re.findall(r"\[Errno -\d+]", str(exc)):
# This exception is raised by the smbprotocol.transport:Tcp.connect
# and originates from socket.gaierror (OSError). These exceptions might
# be raised due to network instability. We will retry to connect.
retried_errors.append(exc)
else:
# All another ValueError exceptions should be raised, as they are not
# related to network issues.
raise exc
except Exception as exc:
# Save the exception and retry to connect. This except might be dropped
# in the future, once all exceptions suited for retry are identified.
retried_errors.append(exc)

if retry < self.register_session_retries - 1:
time.sleep(1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May as well make the retry time configurable and ideally exponential, base * factor**retry

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is great idea. Should the exponential option be also configurable? To avoid confusing a user?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to make the max time equal to the wait time

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an interesting idea, but not the way around it is is other implementations (example). At least, then I would call it "max_wait_time", but probably better to have "base_wait_time" and backoff factor.


# Raise last exception to inform user about the connection issues.
# Note: Should we use ExceptionGroup to raise all exceptions?
raise retried_errors[-1]

@classmethod
def _strip_protocol(cls, path):
Expand Down