1Password / connect-sdk-python

Python SDK for 1Password Connect
https://developer.1password.com/docs/connect
MIT License
200 stars 31 forks source link

[Feat] Allow custom timeout using env vars #94

Closed raphapassini closed 6 months ago

raphapassini commented 8 months ago

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

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

raphapassini commented 6 months ago

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

HaddadJoe commented 6 months ago

nice thanks! any eta on the next release please?

volodymyrZotov commented 6 months ago

@HaddadJoe will be available later today or tomorrow

HaddadJoe commented 6 months ago

@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 commented 6 months ago

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

tblnd commented 6 months ago

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

@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 commented 6 months ago

I've created a PR that should fix the issue

volodymyrZotov commented 6 months ago

Fixed in v1.5.1