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

onboarding: request level timeouts #189

Merged
merged 2 commits into from
Sep 6, 2024
Merged

onboarding: request level timeouts #189

merged 2 commits into from
Sep 6, 2024

Conversation

TalDerei
Copy link
Contributor

@TalDerei TalDerei commented Sep 5, 2024

references #164 (comment) cc @grod220

based on offline discussion with @turbocrime, the channel transport has default 60 second timeout, and grpc web transport have no timeout. The channel transport timeout is insufficient for our needs, as we may need to cycle through multiple rpcs to request the block height during onboarding. Instead, we’ve implemented a more granular request-level timeout of 5 seconds directly within the fetch request.

@TalDerei TalDerei self-assigned this Sep 5, 2024
@TalDerei TalDerei requested review from turbocrime and a team September 5, 2024 04:54
@turbocrime
Copy link
Contributor

based on offline discussion with @turbocrime, the channel transport has default 60 second timeout, and grpc web transport (intra-document requests) have no timeout

i must have been unclear - that internalTransportOptions with 0 timeout is applied only to the transport used by extension pages querying rpc services, and loopback clients within the handlercontext.

none of the createGrpcWebTransport calls within the repository presently apply a timeout configuration, and i'm uncertain of the default as that implementation from a third party. i looked earlier but could not find a default, so it may also be 0, which means it would ultimately be rejected by a browser-level HTTP failure.

kind of irrelevant i guess. reviewing now

Copy link
Contributor

@turbocrime turbocrime Sep 5, 2024

Choose a reason for hiding this comment

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

i think this could be simplified.

notably, each PromiseClient method accepts options as a second parameter, including both timeoutMs and an AbortSignal.

interface CallOptions {
  /**
   * Timeout in milliseconds.
   *
   * Set to <= 0 to disable the default timeout.
   */
  timeoutMs?: number;

  /**
   * An optional AbortSignal to cancel the call.
   * If cancelled, an error with Code.Canceled is raised.
   */
  signal?: AbortSignal;
  
  headers?: HeadersInit;
  onHeader?(headers: Headers): void;
  onTrailer?(trailers: Headers): void;
  contextValues?: ContextValues;
}

pre-onboarding, you won't have access to the normal fullnode queriers from services. so constructing transports is definitely the right idea.

will come back with some suggestions.

Copy link
Contributor

@turbocrime turbocrime left a comment

Choose a reason for hiding this comment

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

suggestions in comments and in #191

todo: the new multi-rpc fetcher seems overcomplicated, so it would be good to simplify.

merge when you're satisfied.

@TalDerei TalDerei merged commit 75af184 into main Sep 6, 2024
3 checks passed
@TalDerei TalDerei deleted the channel-transport branch September 6, 2024 07:30
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.

2 participants