Fix timeout #773
Fix timeout #773
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @juancarlospaco - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
supabase/lib/client_options.py
Outdated
| @@ -36,7 +35,7 @@ class ClientOptions: | |||
|
|
|||
| postgrest_client_timeout: Union[ | |||
| int, float, Timeout | |||
| ] = DEFAULT_POSTGREST_CLIENT_TIMEOUT | |||
| ] = None | |||
There was a problem hiding this comment.
issue (edge_case_not_handled): Setting default timeout to None may lead to indefinite waits.
Consider setting a sensible default timeout to prevent potential issues with hanging connections.
supabase/lib/client_options.py
Outdated
| @@ -55,7 +54,7 @@ def replace( | |||
| realtime: Optional[Dict[str, Any]] = None, | |||
| postgrest_client_timeout: Union[ | |||
| int, float, Timeout | |||
| ] = DEFAULT_POSTGREST_CLIENT_TIMEOUT, | |||
| ] = None, | |||
There was a problem hiding this comment.
issue (code_clarification): Consistency issue with default timeout setting in function parameters.
The default value for 'postgrest_client_timeout' in the 'replace' function is set to None, which is inconsistent with the class attribute default. This might lead to unexpected behavior.
| client.table("sample").insert(data).execute() | ||
|
|
||
|
|
||
| @pytest.mark.xfail( |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for a valid but long timeout value.
It would be beneficial to include a test case that sets a long but valid timeout value to ensure that the system handles extended operations without premature termination.
| @pytest.mark.xfail( | |
| @pytest.mark.xfail( | |
| reason="postgrest client timeout reached before processing the request." | |
| ) | |
| def test_passing_wrong_timeout_value(): | |
| options = ClientOptions(postgrest_client_timeout = 300) # Extended timeout. | |
| client = create_client(url, key, options) | |
| data = [{"item": value} for value in range(0, 1000000)] | |
| client.table("sample").insert(data).execute() |
| import pytest | ||
|
|
||
|
|
||
| def test_new_timeout_configuration(): |
There was a problem hiding this comment.
issue (testing): Test does not verify the effect of the new timeout configuration.
Please add assertions to check that the timeout setting is correctly applied and behaves as expected when set to None.
|
Hey! Personally, I think having |
|
Changing this to |
|
The changes for Postgrest-py should now be included in 2.4.4 that was released earlier. |
What kind of change does this PR introduce?
Fix for timeout-related issues.
According to httpx documentation setting the timeout property to
Nonewill ensure no timeout is used.This could be useful when the user cannot estimate how long a running task should take,
and improves the user experience because avoids using arbitrary values to get rid of such timeout errors.
A test to play with timeouts is included, with happy-path and expected-to-fail scenarios.
See also:
Explicit is better than implicit.
What is the current behavior?
Notes
Another different potential approach can be just increasing the
DEFAULT_POSTGREST_CLIENT_TIMEOUT,a value of
5feels too small for long-running tasks whatsoever.JavaScript client does not have an explicit timeout by default (?), AFAIU.
Some libraries in the wild use
-1or0to mean no explicit timeout or a very huge number likely unreachable.If the desired approach is to use an implicit integer, then maybe using a huge number can be considered.