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

Added default timeout on connection.receive() #257

Merged
merged 3 commits into from
Jan 8, 2024

Conversation

Robbert-Brand
Copy link
Contributor

The library hang when following these steps:

  • Connect using register_connection() to a SMB server.
  • Shut down the server, using the normal shutdown procedure.
  • Call any kind of disconnect(), like reset_connection_cache()
    Now the library hangs.

This is because of the connection.receive() function is called against a dead server, without a timeout.

This pull request suggests setting the default timeout of the connection.receive() function to the same value as the default timeout of the other functions.

Copy link

codecov bot commented Dec 10, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (37512ee) 99.06% compared to head (8538957) 99.06%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #257   +/-   ##
=======================================
  Coverage   99.06%   99.06%           
=======================================
  Files          24       24           
  Lines        5112     5112           
=======================================
  Hits         5064     5064           
  Misses         48       48           
Flag Coverage Δ
99.06% <100.00%> (ø)
py3.10 99.02% <100.00%> (ø)
py3.11 99.02% <100.00%> (ø)
py3.12 99.02% <100.00%> (ø)
py3.7 99.01% <100.00%> (ø)
py3.8 99.02% <100.00%> (ø)
py3.9 99.05% <100.00%> (ø)
x64 99.06% <100.00%> (ø)
x86 98.98% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jborean93
Copy link
Owner

Sorry it's getting to the holiday season so I haven't been able to get to this. An initial impression gives me conerns around changing the default value here as I know some tools rely on it waiting indefinitely for a response, say a pipe read. It'll require some closer investigation to see what the best way forward for this is.

@Robbert-Brand
Copy link
Contributor Author

Perhaps an alternative option would be to pass an (optional) timeout value with the disconnect and other high-level interface functions? That way, the app only hangs when the user wishes it to do so.

@jborean93
Copy link
Owner

I think at least making sure disconnect() doesn't hang forever is a good middle ground for now. It's a pity I painted myself in the corner here with the defaults but what is done is done.

@Robbert-Brand
Copy link
Contributor Author

Hi!
I updated the pull request as you suggested. It is now possible to supply a timeout with the disconnect() functions, and the delete_session that calls the disconnect() functions defaults to the standard timeout of 60 seconds.
I also changed back the receive timeout to None, as it was originally.
Robbert

@@ -989,7 +989,7 @@ def send_compound(self, messages, sid, tid, related=False):
"""
return self._send(messages, session_id=sid, tree_id=tid, related=related)

def receive(self, request, wait=True, timeout=None, resolve_symlinks=True):
def receive(self, request, wait=True, timeout=60, resolve_symlinks=True):
Copy link
Owner

Choose a reason for hiding this comment

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

Did you intend to leave this as 60 and not None as the default?

@Robbert-Brand
Copy link
Contributor Author

Ah, I forgot to change that one back. My apologies, now the default of receive() should be as it originally was.

@jborean93
Copy link
Owner

Thanks for working on this, sorry for the delay in the original review.

@jborean93 jborean93 merged commit 7d6abeb into jborean93:master Jan 8, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants