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

[Feat] Allow custom timeout using env vars #94

Merged
merged 3 commits into from
Apr 2, 2024

Conversation

raphapassini
Copy link
Contributor

For some use cases, for instance, when there's a VPN between the computer running the client and the 1password servers the default timeout may not be enough. In those cases the user receives a httpx.ConnectTimeout exception.

This PR will allow the user to define a custom timeout by using an environment variable.

@HaddadJoe
Copy link
Contributor

@volodymyrZotov quick one, do you mind taking a look. This is really important for us as we're seeing lots of timeouts here

Copy link
Contributor

@volodymyrZotov volodymyrZotov left a comment

Choose a reason for hiding this comment

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

Overall looks good. 👍 Thank you for the contribution! That's good improvement for the package.

Your PR made me think about brother implementation. I filled the issue for that #99 .

I left several small comments to this one PR. Please check them out. And it would be good to be merged.

README.md Outdated Show resolved Hide resolved
src/onepasswordconnectsdk/async_client.py Outdated Show resolved Hide resolved
@raphapassini
Copy link
Contributor Author

Hey @volodymyrZotov thanks for the review. I've done the requested changes. Do you mind doing another review?
Thanks :)

Copy link
Contributor

@michaelAbon1p michaelAbon1p left a comment

Choose a reason for hiding this comment

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

Just reading through the code, not trying it out, I like how the changes look. This will be a useful addition. Thanks for contributing it!

I'll defer to @volodymyrZotov here, but consider me a second thumbs up ✅

Copy link
Contributor

@volodymyrZotov volodymyrZotov left a comment

Choose a reason for hiding this comment

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

Works as expected. The code looks good to me! Approved!

@volodymyrZotov volodymyrZotov merged commit 80afcc1 into 1Password:main Apr 2, 2024
1 check passed
@HaddadJoe
Copy link
Contributor

nice thanks! any eta on the next release please?

@github-actions github-actions bot mentioned this pull request Apr 2, 2024
2 tasks
@volodymyrZotov
Copy link
Contributor

@HaddadJoe will be available later today or tomorrow

@HaddadJoe
Copy link
Contributor

HaddadJoe commented Apr 2, 2024

@volodymyrZotov I think this PR is causing issues. If it's not set the int here sets timeout to 0. The if statement fails because 0 as boolean is false. It then defaults to USE_CLIENT_DEFAULT which is a class. Or when you call that underlying https library does some addition

File "/pypoetry/virtualenvs/Av-py3.11/lib/python3.11/site-packages/anyio/_core/_tasks.py", line 111, in fail_after
deadline = (current_time() + delay) if delay is not None else math.inf
~~~~~~~~~~~~~~~^~~~~~~
TypeError: unsupported operand type(s) for +: 'float' and 'UseClientDefault'

def get_timeout() -> Union[int, UseClientDefault]:
    """Get the timeout to be used in the HTTP Client"""
    timeout = int(os.getenv(ENV_CLIENT_REQUEST_TIMEOUT, 0))
    return timeout if timeout else USE_CLIENT_DEFAULT

as a fix for now, using the timeout works fine but the default scenario, which will be most 1pass users as of now it will fail

@volodymyrZotov
Copy link
Contributor

@HaddadJoe Thank you for letting know! It needs a fix.

@tblnd
Copy link

tblnd commented Apr 3, 2024

Hi team, since this PR was merged, our deployment of the 1Password Connect Server is failing although we have updated our deployment with the new environment variable where the SDK is running: OP_CLIENT_REQUEST_TIMEOUT=90.

The error message is

TypeError: 'UseClientDefault' object cannot be interpreted as an integer

Can you advise on how we can fix this issue please?

@HaddadJoe
Copy link
Contributor

HaddadJoe commented Apr 3, 2024

@tblnd until a fix is released what worked for us is setting OP_CONNECT_CLIENT_REQ_TIMEOUT to something reasonable like 5 or 10 (it's in seconds)

@raphapassini
Copy link
Contributor Author

I've created a PR that should fix the issue

@volodymyrZotov
Copy link
Contributor

Fixed in v1.5.1

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.

5 participants