alloy icon indicating copy to clipboard operation
alloy copied to clipboard

refactor: Refactor ChainStreamPoller

Open popzxc opened this issue 1 year ago • 5 comments
trafficstars

Fixes #1053

This PR turned out a bit larger than I wanted it to be, but I don't think it makes much sense to split it into several ones. Here's what changed:

  • TransportErrorKind::recoverable marked as deprecated, because there already is a more advanced is_retry_err method. The deprecation message suggests using it, or RetryBackoffLayer for HTTP.
  • Retries were removed from PollerBuilder and ChainStreamPoller. The logic there was pretty basic and it wouldn't solve most problems that can realistically occur. IMHO, recovering belongs to the transport domain; otherwise, there will be one recovery implementation per client user, and a good solution is already present in the project - RetryBackoffLayer. I guess something like this is needed for WebSocket as well.
  • ChainPollerStream was split into two parts. One handles initialization (works like a tiny builder), and one actually stores the stream state. It allowed us to get rid of NO_BLOCK_NUMBER used as a placeholder.
  • Dedicated error type was added to simplify implementation.
  • Finally, batches are now requested via JSON RPC batch request instead of sequential single requests.

popzxc avatar Jul 16 '24 11:07 popzxc

@mattsse could you please take a look?

popzxc avatar Jul 22 '24 17:07 popzxc

ah sorry, thanks for the ping, will check in a bit

mattsse avatar Jul 22 '24 17:07 mattsse

GM @mattsse Sorry for pinging again; do you think anything else should be done in the scope of this PR?

popzxc avatar Jul 29 '24 06:07 popzxc

@mattsse this PR got a bit stale :) If this functionality is not something alloy will benefit from, I can close the PR (though I've promised to ship some currently lacking tests that depend on this functionality), but I would prefer not to leave it hanging.

popzxc avatar Aug 09 '24 05:08 popzxc

@DaniPopes @gakonst Sorry for pinging, but I'm unsure how to proceed with this PR.

popzxc avatar Aug 13 '24 11:08 popzxc

I'll close this PR as stale. If you think it's worth merging, I'm happy to re-open and resolve merge conflicts.

popzxc avatar Oct 12 '24 09:10 popzxc