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

Pass Context from enclave rpc to database #1882

Merged
merged 3 commits into from
Apr 23, 2024
Merged

Conversation

tudor-malene
Copy link
Collaborator

@tudor-malene tudor-malene commented Apr 22, 2024

Why this change is needed

Some SQL calls are very slow, and we need a way to cancel them.

The "Ten Gateway" makes all RPC calls to the node with a hardcoded Timeout Context of 5s. This context is propagated through the host to the enclave, and all the way to the database calls.

Note that the EVM db calls are happening outside the context. For those, we use a configuration.

What changes were made as part of this PR

  • Pass the context through the entire stack.
  • Pass the rpc timeout config where it is necessary.
  • Set timeouts on the Ten gateway.

PR checks pre-merging

Please indicate below by ticking the checkbox that you have read and performed the required
PR checks

  • PR checks reviewed and performed

Copy link
Collaborator

@BedrockSquirrel BedrockSquirrel left a comment

Choose a reason for hiding this comment

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

LGTM, optional comment

@@ -67,11 +71,16 @@ func UnauthenticatedTenRPCCall[R any](ctx context.Context, w *Services, cfg *Cac
return withPlainRPCConnection(w, func(client *rpc.Client) (*R, error) {
var resp *R
var err error

basectx := ctx
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we could just return an error if context is nil (if there's no way to enforce not nil). I had a quick glance and it seems like all usages pass in a context.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good idea. simplifies it

Copy link
Contributor

@StefanIliev545 StefanIliev545 left a comment

Choose a reason for hiding this comment

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

Don't see anything unexpected so LGTM

@tudor-malene tudor-malene merged commit 7ca4364 into main Apr 23, 2024
2 checks passed
@tudor-malene tudor-malene deleted the tudor/performance_fixes branch April 23, 2024 13:45
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