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

Allow devs to supply a dispatcher to the Node implementation of fetch #2178

Conversation

steveluscher
Copy link
Collaborator

@steveluscher steveluscher commented Feb 24, 2024

Summary

The current version of web3.js allows folks to supply what's called an ‘HTTP Agent.’ This is a class that mediates how network requests should be fetched. One example is to establish a connection pool for a given URL, and keep connections open for a time so that TLS/SSL negotiation doesn't have to be done on every request.

The new web3.js up until this point has punted on how to reintroduce this concept. In this PR we bring it back in the form of allowing callers to create transports with custom Undici Dispatchers.

It only works in Node. If you supply the dispatcher_NODE_ONLY property in non-Node environments you'll get a console warning that your config has been ignored.

Alternate designs considered

I desperately wanted to avoid the situation in this PR, which was to create a config property on transports that's only usable in Node.

Alternate approaches considered:

  1. Create a special transport for use in Node. Callers would have to createDefaultNodeRpcTransport() and/or solanaRpcFactoryForNode() which I thought was a bridge too far.
  2. Encourage people to inject a global dispatcher. This is a terrible idea for all of the reasons that global state is terrible:
    • Applies to all connections in the process
    • Your global dispatcher can be unset by someone else
    • There might already be a global dispatcher installed (eg. a ProxyAgent) and you might unknowingly replace it
  3. Export a Dispatcher setter that lets you install a dispatcher accessible only to @solana/fetch-impl. Besides having most of the bad features of #​2, this would not work. We inline @solana/fetch-impl in the build, meaning there's no longer anywhere to export such a method from.

Test Plan

cd packages/fetch-impl/
pnpm test:unit:node
cd ../rpc-transport-http/
pnpm test:unit:node

See benchmark tests in next PR.

Closes #2126.

Copy link

socket-security bot commented Feb 24, 2024

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report↗︎

@steveluscher steveluscher force-pushed the 02-22-Ignore_any_test_files_ending_with_-test.node.ts_or_-test.browser.ts_in_other_environments branch from 6ac84da to ef8b86c Compare February 24, 2024 20:41
@steveluscher steveluscher force-pushed the 02-22-Add_a_default_dispatcher_configured_for_keepalive_to_the_Node_implementation_of_fetch_ branch 2 times, most recently from 3911705 to c52409a Compare February 24, 2024 21:00
Comment on lines 6 to 15
cachedDispatcher = new Agent({
// FIXME: https://github.com/nodejs/undici/issues/2808
// allowH2: true,
connections: 16,
// One second fewer than the Solana RPC's keepalive timeout.
// Read more: https://github.com/solana-labs/solana/issues/27859#issuecomment-1340097889
keepAliveTimeout: 19000,
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I made the decision to deliver a default dispatcher to everyone. This is what the current web3.js does. I really hate this, because everyone's workload is different and requires tuning, but it's also true that having no dispatching strategy has pretty awful performance.

@steveluscher steveluscher force-pushed the 02-22-Add_a_default_dispatcher_configured_for_keepalive_to_the_Node_implementation_of_fetch_ branch from c52409a to baeabbe Compare February 24, 2024 21:06
@steveluscher steveluscher force-pushed the 02-22-Ignore_any_test_files_ending_with_-test.node.ts_or_-test.browser.ts_in_other_environments branch from ef8b86c to 4cac623 Compare February 24, 2024 21:10
Base automatically changed from 02-22-Ignore_any_test_files_ending_with_-test.node.ts_or_-test.browser.ts_in_other_environments to master February 24, 2024 21:12
@steveluscher steveluscher force-pushed the 02-22-Add_a_default_dispatcher_configured_for_keepalive_to_the_Node_implementation_of_fetch_ branch 2 times, most recently from 3534b9b to 80012ba Compare February 24, 2024 21:21
@lorisleiva
Copy link
Collaborator

Is there no way to bring this Dispatcher API to the browser?

@steveluscher
Copy link
Collaborator Author

There is not. The browser doesn't let you tune its networking stack.

@steveluscher steveluscher force-pushed the 02-22-Add_a_default_dispatcher_configured_for_keepalive_to_the_Node_implementation_of_fetch_ branch from 80012ba to f6cf535 Compare February 27, 2024 01:21
// allowH2: true,
// See https://github.com/solana-labs/solana-web3.js/pull/2179 for
// strategies on how to tune the number of connections through benchmarking.
connections: 32,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm starting to regret this in light of my latest benchmarks in #2179.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we're not creating a more performant Agent by providing these opinionated default values then that's good right? At least we don't need to be opinionated here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I think I might tear this default Agent out until we have evidence that undici with no dispatcher is insufficient. The getLatestBlockhash benchmarks would indicate that it does a fine job of scheduling requests without one.

// allowH2: true,
// See https://github.com/solana-labs/solana-web3.js/pull/2179 for
// strategies on how to tune the number of connections through benchmarking.
connections: 32,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we're not creating a more performant Agent by providing these opinionated default values then that's good right? At least we don't need to be opinionated here.

@steveluscher steveluscher force-pushed the 02-22-Add_a_default_dispatcher_configured_for_keepalive_to_the_Node_implementation_of_fetch_ branch from f6cf535 to 281265a Compare February 27, 2024 18:55
@steveluscher steveluscher changed the title Add a default dispatcher configured for keepalive to the Node implementation of fetch Allow devs to supply a dispatcher to the Node implementation of fetch Feb 27, 2024
@steveluscher
Copy link
Collaborator Author

steveluscher commented Feb 27, 2024

Merge activity

@steveluscher steveluscher merged commit a2fc5a3 into master Feb 27, 2024
5 of 7 checks passed
@steveluscher steveluscher deleted the 02-22-Add_a_default_dispatcher_configured_for_keepalive_to_the_Node_implementation_of_fetch_ branch February 27, 2024 18:58
Copy link
Contributor

github-actions bot commented Mar 2, 2024

🎉 This PR is included in version 1.90.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link
Contributor

Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[experimental] Figure out recommended way to override fetch dispatcher in Node
2 participants