Add connection timeout conf option

[iamjackg]

I ran into issues with the component getting completely stuck and preventing all calendars from updating if the connection never completed. The default timeout is no timeout, so the data lock would never be released, and all calendars would stop updating. This PR adds a configuration parameter to change this, which defaults to "no timeout" for backwards compatibility.

[franc6]

I'm pretty sure I've seen timeout errors in the past, so I'm not sure why you're seeing it hang forever. However, adding the feature is good, anyway. Please change the default from an explicit None to the default timeout (see https://github.com/python/cpython/issues/62617, especially the last comment). Also, unit tests, and updates to the existing unit tests are needed, too. Finally, since timeout is a whole number of seconds, the type hint should be "int", not "float".

Thanks!

[iamjackg]

My bad, I completely missed the existence of the tests folder :) Working on that.

As for your other points, in cpython the default timeout is None, and it is a float. See: https://docs.python.org/3/library/socket.html#socket.setdefaulttimeout

I can take a different approach where I build a dict of kwargs to pass to urlopen containing timeout if CONF_CONNECTION_TIMEOUT is not None, so we can sidestep the issue altogether.

EDIT: forgot that the built-in Home Assistant validator I'm using (https://github.com/home-assistant/core/blob/f45f0b432732c0b664ed6a0a712009c1c72de9f8/homeassistant/helpers/config_validation.py#L754) also coerces None to _GLOBAL_DEFAULT_TIMEOUT, further reinforcing that it's fine as default.

[franc6]

I see, regarding the type. That’s just weird — I can guess why, but the precision loss of float (double) just makes it a weird choice to me. I’d still rather use socket.getdefaulttimeout, just in case the default changes — it’s not that I think it’s likely, it’s just more tidy, in my opinion. :)

[iamjackg]

Sorry, I might have edited my response after you already wrote yours -- the issue is actually sidestepped altogether by using Home Assistant's own cv.socket_timeout validator. The value we get in the config object will always be either a specified timeout or the default, since the validator coerces it correctly. I know that, if you look at the implementation, they're also using the internal variable directly instead of calling getdefaulttimeout, but in this kind of ecosystem, I think the most important thing is being consistent with other integrations and using the hass builtin utilities as much as we can.

The Pioneer integration is also using the same pattern (with None as default), and so is the modbus component.

Anyway, I refactored all the tests and added one for the timeout case, although it's really just for show since the exception that's raised for timeouts is also a URLError. :laughing:

[franc6]

Thanks for the updates. I'll merge this before I make another release, but I'm gonna make some changes to the unit tests, and update the README.md & info.md files, too. Oh, and if you submit another PR, please don't ignore the PR template. :)

[codecov-commenter]

Codecov Report

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

Comparison is base (39d0157) 97.69% compared to head (b9ae831) 97.70%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## releases #114 +/- ## ============================================ + Coverage 97.69% 97.70% +0.01% ============================================ Files 9 9 Lines 434 436 +2 Branches 75 75 ============================================ + Hits 424 426 +2 Misses 4 4 Partials 6 6 ```

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

[iamjackg]

Yeah, sorry for not following the template (edit: GitHub's web editor never showed it to me). I had fixed this for myself with a manual patch, then I figured it might have been useful to others too, so I snuck it in in a rare moment of free time. Thanks for carrying it past the finish line.

[iamjackg]

(for the record, I submitted the first version through GitHub's web editor, and when committing you can ask it to automatically create a pull request for you. As it turns out, that skips the PR template altogether: I never saw one. Something to keep in mind for the future)

[franc6]

Thanks for the heads-up on that. It’s good to know for other projects. On Dec 28, 2023, at 3:04 PM, Jack Gaino @.***> wrote: (for the record, I submitted the first version through GitHub's web editor, and when committing you can ask it to automatically create a pull request for you. As it turns out, that's skips the PR template altogether: I never saw one. Something to keep in mind for the future)

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: @.***>