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: add timeout to did resolver resolve method #2464

Merged
merged 2 commits into from
Sep 6, 2023

Conversation

dbluhm
Copy link
Contributor

@dbluhm dbluhm commented Sep 6, 2023

This PR adds a timeout parameter to the DID Resolver interface, defaulting to 30 seconds. Without it, a poorly behaved resolver plugin can "resolve" documents that aren't actually DID Docs and cause ACA-Py to run out of memory.

There's space for additional tuning on the timeout default. Some DID Methods are, in my experience, quite slow (did:btcr and did:ion resolvers can be pretty slow sometimes). 30 seconds might be overly charitable.

@@ -42,20 +43,28 @@ async def _resolve(
profile: Profile,
did: Union[str, DID],
service_accept: Optional[Sequence[Text]] = None,
*,
timeout: Optional[int] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

why not initialize to 30 instead of None? That would make it more evident that the time-out default is 30 seconds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair question; using the optional enables expression that the timeout wasn't set. Since we call _resolve from two other methods, and we want the timeout to apply to both and we want a default of 30 seconds when the timeout isn't set, we would have to set the default timeout to 30 on both the resolve and resolve_with_metadata methods or else make sure the timeout wasn't included in the call to _resolve. It ends up being cleaner to set the default when timeout is None. But I think there's a middle ground. I'll put a constant on the class that has the default timeout value so it's clearer what it is and how it's used at a glance.

burdettadam
burdettadam previously approved these changes Sep 6, 2023
Copy link
Contributor

@usingtechnology usingtechnology left a comment

Choose a reason for hiding this comment

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

LGTM.
Your logic around None vs 30 makes sense, so works for me.

@dbluhm dbluhm force-pushed the fix/did-resolver-timeouts branch from 0690ad6 to 6ea611f Compare September 6, 2023 18:07
@dbluhm dbluhm enabled auto-merge September 6, 2023 18:08
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@dbluhm dbluhm merged commit fc7f748 into openwallet-foundation:main Sep 6, 2023
@dbluhm dbluhm deleted the fix/did-resolver-timeouts branch January 30, 2024 21:32
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.

3 participants