fix abort controller handling #36

joewagner · 2023-09-24T23:30:31Z

The initial work for this PR is located in https://github.com/tablelandnetwork/js-tableland/pull/576/files

Summary
Fixes abort controller handling which allows a user to abort and configure the polling timeout and interval for SDK methods that poll the gateway for a receipt.

Details
The core of this PR was adding the wiring to make the underlying polling handling aware of the user's abort controller. However, I found the API to be unintuitive and spent some time refactoring how a user goes about providing a abort-able controller with a custom timeout and polling interval. See comments below for details.

NOTE: This would be a breaking API change and therefore would require a major version bump. We probably could make the core fix while not breaking the API, but I don't think there's any need to be bashful about frequently releasing new major versions at this point. What do you guys think?

sanderpick · 2023-10-09T19:23:03Z

Thanks for sorting this out @joewagner. Should we get this out soonish? Sounded like the timeout issue was blocking some folks on Filecoin.

joewagner · 2023-10-10T05:42:27Z

Thanks for sorting this out @joewagner. Should we get this out soonish? Sounded like the timeout issue was blocking some folks on Filecoin.

Sounds good to me! I'm planning to make sure the studio users get support, but otherwise can work on this.

sanderpick · 2023-10-18T20:42:12Z

@joewagner I asked @dtbuchholz if he could take this over... sounds like the abort bug is the main cause of pain for the hackathon so figured we should try to get this out.

dtbuchholz · 2023-10-19T07:31:26Z

packages/sdk/src/database.ts

-          statements.map(async (stmt) => await stmt.all<T>(undefined, opts))
+          statements.map(
+            async (stmt) => await stmt.all<T>(undefined, controller)
+          )


@sanderpick from this comment—i don't think this is a problem? i tested it out like this, if it's what you meant:

const batch = await db.batch( [ db.prepare(`SELECT * FROM ${tableOne};`), db.prepare(`SELECT * FROM ${tableTwo};`), db.prepare(`SELECT * FROM ${tableThree};`), db.prepare(`SELECT * FROM ${tableFour};`), db.prepare(`SELECT * FROM ${tableFive};`), ], controller );

and i get the array back w/o issue:

[ { meta: { duration: 50.153499603271484 }, success: true, results: [ [Object] ] }, { meta: { duration: 48.47224998474121 }, success: true, results: [ [Object] ] } ]

i can add a test but double checking my understanding.

cool, trying to refresh my memory here. shouldn't your array have 5 entries?

dtbuchholz · 2023-10-19T07:40:03Z

two other follow ups:

regarding this comment, is it something worthwhile implementing, or should we keep this as-is?
in order to set a predefined per-chain polling timeout value, would it make sense to put that in evm-tableland's network.ts file? otherwise, it'd require custom chain handling in the chains helper, which tries to avoid that by importing from evm-tableland. for example, network.ts could export a "chainPollingTimeout" or "chainBlockTime"...something like that.

dtbuchholz · 2023-10-19T21:55:43Z

i tested these changes in the Studio with just a symlink (no code changes to the Studio...and just using the default controller functionality in this PR). occasionally, it works on sepolia, but it's still aborting. in this video, you'll notice:

the tx is signed, the SDK starts waiting for confirmation, and it gets confirmed onchain ~20 seconds later—the tx is logged in the console.
thereafter, it takes 13 seconds for the validator to materialize the table. the SDK makes 8 requests without success and then aborts. (you can see on the far right, i'm manually refreshing the /tables endpoint until this table shows up.)

it seems like the validator is having issues materializing sepolia data?

Screen.Recording.2023-10-19.at.2.33.14.PM.mov

sanderpick · 2023-10-20T00:53:37Z

regarding this comment, is it something worthwhile implementing, or should we keep this as-is?

hmm, i'm not totally clear on what needs to conform to the D1 API, and if we're planning on maintaining compatibility. my pref would be to make an options object like in my example commit. @joewagner do you have any insights here?

in order to set a predefined per-chain polling timeout value, would it make sense to put that in evm-tableland's network.ts file? otherwise, it'd require custom chain handling in the chains helper, which tries to avoid that by importing from evm-tableland. for example, network.ts could export a "chainPollingTimeout" or "chainBlockTime"...something like that.

Nice, I like the idea of using evm-tableland here. That could be integrated here in a diff PR.

sanderpick · 2023-10-20T00:59:22Z

@dtbuchholz regarding the video, if you increase the timeout to like 60 seconds, it should work then right? ignoring the fact that it shouldn't be that slow on sepolia for now (@avichalp / @brunocalza any idea what's going on there? 13 seconds sounds pretty slow.)

joewagner · 2023-10-20T07:38:03Z

regarding this comment, is it something worthwhile implementing, or should we keep this as-is?

hmm, i'm not totally clear on what needs to conform to the D1 API, and if we're planning on maintaining compatibility. my pref would be to make an options object like in my example commit. @joewagner do you have any insights here?

Based on the abstract class D1 publishes, it looks like we need to make the first method polymorphic and potentially accept a colName string parameter, but it can also be called with no arguments.
This means we should be able to accept an optional Options that contains whatever properties we want to support in all of the statement methods.
maybe something like:

{
  first<T = unknown>(colName: string, opts?: TablelandOptions): Promise<T | null>;
  first<T = Record<string, unknown>>(opts?: TablelandOptions): Promise<T | null>;
  run<T = Record<string, unknown>>(opts?: TablelandOptions): Promise<D1Result<T>>;
  all<T = Record<string, unknown>>(opts?: TablelandOptions): Promise<D1Result<T>>;
  raw<T = unknown[]>(opts?: TablelandOptions): Promise<T[]>;
}

Any kind of third party tools that expect the SDK to be D1 compliant will be able to call those methods as ewpected, and we can still allow folks to supply an options object that indicates the timeout they want.

NOTE: looks like we currently do (opts: SignalAndInterval = {}) instead of using the ?, i.e. (opts?: SignalAndInterval). Our current way of setting a default feels nicer and has the same affact imo, but either should work.

joewagner · 2023-10-20T07:47:09Z

I have a thought after looking at this with fresh eyes. Regardless if we go with per chain default timeouts, it would be nice to be able to set a global timeout via an overrideDefaults[fixed link] type of interface.
This way, as a dev, I can decide how often my default polling should happen and how long to wait before stopping the polling.

brunocalza · 2023-10-20T12:34:41Z

@dtbuchholz regarding the video, if you increase the timeout to like 60 seconds, it should work then right? ignoring the fact that it shouldn't be that slow on sepolia for now (@avichalp / @brunocalza any idea what's going on there? 13 seconds sounds pretty slow.)

According to the current config , we're waiting for at least one block to consider the tx final. and the ethereum block time is 12s, so I guess that's kind of expected. Were we experiencing faster times before?

dtbuchholz · 2023-10-20T17:11:53Z

According to the current config , we're waiting for at least one block to consider the tx final. and the ethereum block time is 12s, so I guess that's kind of expected. Were we experiencing faster times before?

@brunocalza ah okay that makes sense (i dont think they were faster before...just never really noticed it). out of curiosity, what's the a reason we impose the 1 block depth for sepolia and mumbai when arb/op goerli use 0? do sepolia/mumbai commonly have reorgs or something?

sanderpick · 2023-10-20T17:55:06Z

Any kind of third party tools that expect the SDK to be D1 compliant will be able to call those methods as ewpected, and we can still allow folks to supply an options object that indicates the timeout they want.

@joewagner sounds good. @dtbuchholz do you want to give this a shot?

NOTE: looks like we currently do (opts: SignalAndInterval = {}) instead of using the ?, i.e. (opts?: SignalAndInterval). Our current way of setting a default feels nicer and has the same affact imo, but either should work.

There was some discussion of this here. Personally I prefer the ? for objects that don't have optional params. However, if we implement a true opts object on the top-level API (like here), then it would have optional values and using the default pattern ( = {}) makes sense to me, but internal methods that take controller would keep the ? pattern because that object doesn't have optional params.

I have a thought after looking at this with fresh eyes. Regardless if we go with per chain default timeouts, it would be nice to be able to set a global timeout via an #36 type of interface.
This way, as a dev, I can decide how often my default polling should happen and how long to wait before stopping the polling.

Sounds good! Did you mean to link back to this PR? Should this come in this PR, or as a follow on?

dtbuchholz · 2023-10-20T18:11:42Z

@sanderpick sgtm, will take a stab. and wrt the overrideDefaults, i think it's referring to this.

brunocalza · 2023-10-20T18:29:05Z

According to the current config , we're waiting for at least one block to consider the tx final. and the ethereum block time is 12s, so I guess that's kind of expected. Were we experiencing faster times before?

@brunocalza ah okay that makes sense (i dont think they were faster before...just never really noticed it). out of curiosity, what's the a reason we impose the 1 block depth for sepolia and mumbai when arb/op goerli use 0? do sepolia/mumbai commonly have reorgs or something?

i think it's because reorgs can't happen on L2s

joewagner · 2023-10-20T19:40:42Z

Sounds good! Did you mean to link back to this PR? Should this come in this PR, or as a follow on?

Oops, no I meant to link the overideDefaults method.

tableland-js/packages/sdk/src/helpers/chains.ts

Line 120 in 19582eb

export function overrideDefaults(

sanderpick · 2023-10-20T21:23:48Z

Sweet, @dtbuchholz feel free to try and implement something like that here

…ller

dtbuchholz · 2023-10-28T01:37:56Z

packages/sdk/src/helpers/await.ts

+  return {
+    signal: controller.signal,
+    abort: () => {
+      clearTimeout(timeoutId);


Note: the only change I made here from Sander's original code was the addition of this clearTimeout(timeoutId) line, to make sure that it's cleared if an abort occurs.

dtbuchholz · 2023-10-28T01:46:49Z

packages/sdk/src/helpers/chains.ts

 */
 export interface ChainInfo {
  chainName: ChainName;
  chainId: number;
  contractAddress: string;
  baseUrl: string;
+  pollingTimeout: number;
+  pollingInterval: number;


I figured that since overrideDefaults is already storing validator-specific and chain information, we could just include the validator polling info there. If needed, I can separate it out into its own interface.

dtbuchholz · 2023-10-28T01:49:46Z

packages/sdk/src/helpers/chains.ts

 const entries = Object.entries(proxies) as Array<[ChainName, string]>;
 const mapped = entries.map(([chainName, contractAddress]) => {
+  // @ts-expect-error this imported object's values are always a string


Because the TablelandNetworkConfig can now have types of string | number, the compiler was complaining here with the new URL constructor and baseURIs[chainName]. I tried a few different fixes like checking types & throwing etc, but nothing worked. I think it's safe to ignore since we know that baseURIs will always be strings, though.

dtbuchholz · 2023-10-28T01:53:00Z

packages/sdk/src/helpers/chains.ts

+  // Use per-chain validator polling timeout period
+  const pollingTimeout = validatorPollingTimeouts[chainName];
+  // Default to 1500ms polling interval, except for Filecoin due to long block times
+  const pollingInterval = chainName.includes("filecoin") ? 5000 : 1500;


For each chain, we take a chain-specific validator polling timeout as defined here, and then we can use that polling timeout by default in other methods. For the interval, I've set the frequency to 1500ms for all chains—except for Filecoin.

Since it can take a few minutes for the validator to materialize FIL data, the standard interval is, instead, 5 seconds...it could be even longer, if needed.

dtbuchholz · 2023-10-28T01:53:46Z

packages/sdk/src/helpers/chains.ts

+ * @param chainNameOrId The requested chain name.
+ * @returns A {@link PollingController} with standard timeout & interval per-chain.
+ */
+export function getChainPollingController(


This method makes it easy to use the chain-specific validator polling timeouts/intervals. It'll create a new polling controller with those chain-specific settings.

packages/sdk/src/statement.ts

dtbuchholz · 2023-10-28T01:57:51Z

packages/sdk/src/statement.ts

-    colName?: K,
-    opts: SignalAndInterval = {}
-  ): Promise<Result<T | T[K]>> {
+  async all<T = S>(opts: Options = {}): Promise<Result<T>> {


Here's an example of how the options are passed.

Note that I had started down the path of D1 API conformance in this PR, but I ended up creating a separate branch for this work. Looks like I removed some stuff on accident. Thus, the removed code might need to be added back, unless that PR gets merged into this one. For example, I removed the colName since it's not part of the D1 API, but that might actually be something we want to keep in there.

sounds good. I don't quite follow the point about removed code, but if easier, you could just target this branch. generally speaking though, its easier to keep PRs small and specific.

packages/sdk/src/statement.ts

packages/sdk/src/validator/receipt.ts

dtbuchholz · 2023-10-28T02:06:01Z

packages/sdk/test/statement.test.ts

      strictEqual(meta.txn, undefined);
      strictEqual(error, undefined);
      assert(meta.duration != null);
-      deepStrictEqual(results, [1, 2, 3, 4]);
+      deepStrictEqual(results, [


As noted, I started down the path of D1 API conformance and looks like I muddied up a bit here. So, the colName can't be passed to all() anymore. Again, I can revert this change here if we want to keep things simple and/or still want that feature. The PR with D1 changes, for reference.

I see. So maybe we can just merge this PR and then you can rebase the other on and fix it up before a major release?

yeah, that makes sense to me.

also tagging @joewagner to help address one of your questions above about the removed code since he was helping with some cleanups for the D1 conformance. do the changes in this comment make sense? namely, this is the current SDK code, before this PR and any D1 conformance:

async all<T = S, K extends keyof T = keyof T>( colName?: undefined, opts?: SignalAndInterval ): Promise<Result<T>>; async all<T = S, K extends keyof T = keyof T>( colName: K, opts?: SignalAndInterval ): Promise<Result<T[K]>>; async all<T = S, K extends keyof T = keyof T>( colName?: K, opts: SignalAndInterval = {} ): Promise<Result<T | T[K]>

and with the changes in this PR & D1 conformance (#63), we dropped colName, and it now simply looks like this:

async all<T = Record<string, S>>(opts: Options = {}): Promise<Result<T>>

similarly, the first method currently looks like this, before any changes from those PRs:

async first<T = S, K extends keyof T = keyof T>(): Promise<T>; async first<T = S, K extends keyof T = keyof T>( colName: undefined, opts?: SignalAndInterval ): Promise<T>; async first<T = S, K extends keyof T = keyof T>( colName: K, opts?: SignalAndInterval ): Promise<T[K] | null>; async first<T = S, K extends keyof T = keyof T>( colName?: K, opts: SignalAndInterval = {} ): Promise<T | T[K] | null>

and after these PR changes:

async first<T = Record<string, S>>(opts?: Options): Promise<T | null>; async first<T = S, K extends keyof T = keyof T>( colName: K, opts?: Options ): Promise<T[K] | null>; async first<T = S, K extends keyof T = keyof T>( colName?: K, opts: Options = {} ): Promise<T | T[K] | null>

the main callout with first is that i dropped the overloading with colName: undefined (primarily because chatgpt didn't see why it was useful, but the return type is a bit different). and if no colName is specified at all, we use the Record/return types from the new D1 API.

anyways, i can just merge this PR and make any potential fixes in the conformance PR...just wanna double check here first so that i dont lose track of things

Making sure I'm on the right page here, are we trying to match these types? https://github.com/cloudflare/workerd/blob/main/types/defines/d1.d.ts

Without trying it, would the colName: undefined let you call first like this: db.prepare(...).first(colName, options);
Where colName is of type string | undefined?

correct, i was just trying to match those types.

so, when i test out your example, it looks like we do need to keep the colName: undefined overload...i'll revert/add it back in #63 and include more test cases. ~~but also...the D1 types use first<T = unknown>(colName: string), so maybe colName: undefined shouldn't be allowed because they require string?~~

edit: i think i misunderstood how generics worked with overloading

wrt your example: what's interesting is that if i don't specify the opts/controller, we dont need the colName: undefined overload; it'll execute the same, whether or not that overload definition is included. but if we do use opts/controller, then it fails, and it only works if i add the colName: undefined overload back in there. i'll debug a bit more, too.

// when the `colName: undefined` overload is not included let c: string | undefined; c = undefined; let row = await stmt.first<any>(c); // works row = await stmt.first<any>(c, { controller }); // fails // error: Argument of type 'undefined' is not assignable to parameter of type 'string | number | symbol'. // The call would have succeeded against this implementation, but implementation signatures of overloads are not externally visible.

@dtbuchholz I'm ok adding it back, or not, up to you.
To put it all in one place... If we can support the following signatures, that would make sense to me:

// needed for ORM await stmt.first(); // needed for ORM, i think? await stmt.first(col); // needed for users who want to modify there timeouts and specify a col await stmt.first(col, opts); // NOTE: I don't know if we **must** allow `col` to be undefined, i'm ok either way. // needed for users who want to modify there timeouts await stmt.first(opts);

sanderpick

Very nice... thanks for adding all the extra tests.

sanderpick · 2023-10-31T15:42:05Z

packages/sdk/test/statement.test.ts

      strictEqual(meta.txn, undefined);
      strictEqual(error, undefined);
      assert(meta.duration != null);
-      deepStrictEqual(results, [1, 2, 3, 4]);
+      deepStrictEqual(results, [


I see. So maybe we can just merge this PR and then you can rebase the other on and fix it up before a major release?

joewagner force-pushed the joe/fix-polling-opts branch from fcf82e4 to 31fffc6 Compare September 25, 2023 02:13

dtbuchholz reviewed Oct 19, 2023

View reviewed changes

dtbuchholz mentioned this pull request Oct 24, 2023

feat: change TablelandDeployments to use registry impl, not interface tablelandnetwork/evm-tableland#448

Merged

8 tasks

joewagner and others added 11 commits October 27, 2023 16:25

(sdk) port sander's pr from js-tableland repo

9224365

(sdk) lint fix

86b6b18

refactor(sdk): add clearTimeout(), timeout val to createPollingContro…

e50737c

…ller

feat(sdk): add per-chain polling info & controller creation

8196e08

feat(sdk): use per-chain controller for receipt

f29e6ce

test(sdk): add generic & chain-specific polling controller

8101c37

fix(sdk): D1 API compatability; use opts for db/stmt

1f310cc

test(sdk): adjust for D1 API changes

114fda9

docs(sdk): update example with polling controller

4c61b9a

docs(sdk): add context for polling & more docs for module exports

5418495

refactor(sdk): use default opts pattern like Options = {}

fa9265e

dtbuchholz added 4 commits October 27, 2023 16:25

test(sdk): add createSignal() checks

c1c75f7

chore(sdk): bump @tableland/evm

1fb2508

refactor(sdk): use import for polling timeouts over hardcoded

e566192

docs(sdk): tweak polling controller language

93d2ade

dtbuchholz force-pushed the joe/fix-polling-opts branch from 31fffc6 to 93d2ade Compare October 28, 2023 00:45

dtbuchholz mentioned this pull request Oct 28, 2023

refactor: D1 API conformance #63

Merged

8 tasks

dtbuchholz reviewed Oct 28, 2023

View reviewed changes

packages/sdk/src/statement.ts Show resolved Hide resolved

dtbuchholz reviewed Oct 28, 2023

View reviewed changes

packages/sdk/src/statement.ts Show resolved Hide resolved

dtbuchholz reviewed Oct 28, 2023

View reviewed changes

packages/sdk/src/validator/receipt.ts Show resolved Hide resolved

dtbuchholz reviewed Oct 28, 2023

View reviewed changes

dtbuchholz marked this pull request as ready for review October 28, 2023 02:10

dtbuchholz requested a review from carsonfarmer as a code owner October 28, 2023 02:10

dtbuchholz mentioned this pull request Oct 28, 2023

refactor(network): change base uri, removing tables endpoint tablelandnetwork/evm-tableland#456

Closed

8 tasks

sanderpick approved these changes Oct 31, 2023

View reviewed changes

dtbuchholz merged commit 8c23061 into main Nov 1, 2023
4 checks passed

dtbuchholz deleted the joe/fix-polling-opts branch November 2, 2023 00:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix abort controller handling #36

fix abort controller handling #36

joewagner commented Sep 24, 2023 •

edited

Loading

sanderpick commented Oct 9, 2023

joewagner commented Oct 10, 2023

sanderpick commented Oct 18, 2023

dtbuchholz Oct 19, 2023

sanderpick Oct 20, 2023

dtbuchholz commented Oct 19, 2023

dtbuchholz commented Oct 19, 2023 •

edited

Loading

sanderpick commented Oct 20, 2023 •

edited

Loading

sanderpick commented Oct 20, 2023

joewagner commented Oct 20, 2023

joewagner commented Oct 20, 2023 •

edited

Loading

brunocalza commented Oct 20, 2023

dtbuchholz commented Oct 20, 2023 •

edited

Loading

sanderpick commented Oct 20, 2023

dtbuchholz commented Oct 20, 2023

brunocalza commented Oct 20, 2023

joewagner commented Oct 20, 2023

sanderpick commented Oct 20, 2023

dtbuchholz Oct 28, 2023

dtbuchholz Oct 28, 2023

dtbuchholz Oct 28, 2023

dtbuchholz Oct 28, 2023

dtbuchholz Oct 28, 2023

dtbuchholz Oct 28, 2023 •

edited

Loading

sanderpick Oct 31, 2023

dtbuchholz Oct 28, 2023

sanderpick Oct 31, 2023

dtbuchholz Oct 31, 2023 •

edited

Loading

joewagner Oct 31, 2023

dtbuchholz Oct 31, 2023 •

edited

Loading

joewagner Nov 1, 2023

sanderpick left a comment

sanderpick Oct 31, 2023

fix abort controller handling #36

fix abort controller handling #36

Conversation

joewagner commented Sep 24, 2023 • edited Loading

sanderpick commented Oct 9, 2023

joewagner commented Oct 10, 2023

sanderpick commented Oct 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtbuchholz commented Oct 19, 2023

dtbuchholz commented Oct 19, 2023 • edited Loading

sanderpick commented Oct 20, 2023 • edited Loading

sanderpick commented Oct 20, 2023

joewagner commented Oct 20, 2023

joewagner commented Oct 20, 2023 • edited Loading

brunocalza commented Oct 20, 2023

dtbuchholz commented Oct 20, 2023 • edited Loading

sanderpick commented Oct 20, 2023

dtbuchholz commented Oct 20, 2023

brunocalza commented Oct 20, 2023

joewagner commented Oct 20, 2023

sanderpick commented Oct 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtbuchholz Oct 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtbuchholz Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtbuchholz Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanderpick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joewagner commented Sep 24, 2023 •

edited

Loading

dtbuchholz commented Oct 19, 2023 •

edited

Loading

sanderpick commented Oct 20, 2023 •

edited

Loading

joewagner commented Oct 20, 2023 •

edited

Loading

dtbuchholz commented Oct 20, 2023 •

edited

Loading

dtbuchholz Oct 28, 2023 •

edited

Loading

dtbuchholz Oct 31, 2023 •

edited

Loading

dtbuchholz Oct 31, 2023 •

edited

Loading