jborean93 / smbprotocol

Python SMBv2 and v3 Client
MIT License
320 stars 73 forks source link

Added default timeout on connection.receive() #257

Closed Robbert-Brand closed 9 months ago

Robbert-Brand commented 10 months ago

The library hang when following these steps:

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.

codecov[bot] commented 10 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #257 +/- ## ======================================= Coverage 99.06% 99.06% ======================================= Files 24 24 Lines 5112 5112 ======================================= Hits 5064 5064 Misses 48 48 ``` | [Flag](https://app.codecov.io/gh/jborean93/smbprotocol/pull/257/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jordan+Borean) | Coverage Δ | | |---|---|---| | [](https://app.codecov.io/gh/jborean93/smbprotocol/pull/257/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jordan+Borean) | `99.06% <100.00%> (ø)` | | | [py3.10](https://app.codecov.io/gh/jborean93/smbprotocol/pull/257/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jordan+Borean) | `99.02% <100.00%> (ø)` | | | [py3.11](https://app.codecov.io/gh/jborean93/smbprotocol/pull/257/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jordan+Borean) | `99.02% <100.00%> (ø)` | | | [py3.12](https://app.codecov.io/gh/jborean93/smbprotocol/pull/257/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jordan+Borean) | `99.02% <100.00%> (ø)` | | | [py3.7](https://app.codecov.io/gh/jborean93/smbprotocol/pull/257/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jordan+Borean) | `99.01% <100.00%> (ø)` | | | [py3.8](https://app.codecov.io/gh/jborean93/smbprotocol/pull/257/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jordan+Borean) | `99.02% <100.00%> (ø)` | | | [py3.9](https://app.codecov.io/gh/jborean93/smbprotocol/pull/257/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jordan+Borean) | `99.05% <100.00%> (ø)` | | | [x64](https://app.codecov.io/gh/jborean93/smbprotocol/pull/257/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jordan+Borean) | `99.06% <100.00%> (ø)` | | | [x86](https://app.codecov.io/gh/jborean93/smbprotocol/pull/257/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jordan+Borean) | `98.98% <100.00%> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Jordan+Borean#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jborean93 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

Robbert-Brand commented 9 months ago

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

jborean93 commented 9 months ago

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