fix abort controller handling by sanderpick · Pull Request #576 · tablelandnetwork/js-tableland

sanderpick · 2023-07-29T00:27:38Z

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?

How it was tested

Updated tests to use the new API.

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

Signed-off-by: Sander Pick <sanderpick@gmail.com>

sanderpick

@joewagner @carsonfarmer here's a draft fix for the polling behavior. Let me know what you think.

sanderpick · 2023-07-31T23:58:35Z

-const controller = new AbortController();
-const signal = controller.signal;
+mport { helpers } from "@tableland/sdk";
+const controller = helpers.createPollingController(60_000, 1500); // polling timeout and interval


Here's a good view of the API. Use a helper method to create a "controller" that is passed to the .all, .run, etc. methods. The user may use this controller to abort the request via its abort method.

sanderpick · 2023-08-01T00:01:51Z

    statements: Statement[],
-    opts: Signal = {}
-    // reads returns an Array with legnth equal to the number of batched statements,
+    controller?: PollingController


Couple things here:

This wasn't really an "options" object, it was a specific type of object. I usually think of "options" as an object that mixes different type of options, e.g., color, maxHeight, etc. I changed the param name to reflect.

The value of opts was Signal, but this method may call mutable methods that take polling options.

sanderpick · 2023-08-01T00:05:03Z

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


I'm guessing this won't work... the timeout would be cancelled after the first result is returned. Sound right? This might need some special handling... ie, create a new controller for each statement (clone of controller) and control each of those with the single user-provider one. I can take a stab if sounds right.

sanderpick · 2023-08-01T00:05:48Z

 }

-export type SignalAndInterval = Signal & Interval;
+export type PollingController = Signal & Interval;


Changing this to a name that more closely describes what it does. I had to dig into the source code to figure this out when I first saw this.

sanderpick · 2023-08-01T00:06:11Z

-    abortSignal = controller.signal;
-    // return the timeoutId so the caller can cleanup
-    timeoutId = setTimeout(function () {
+export function createSignal(): Signal {


Helper method for use with read-only APIs

sanderpick · 2023-08-01T00:06:57Z

+  const timeoutId = setTimeout(function () {
+    controller.abort();
+  }, timeout);
+  return {


This object doubles as an options object for fetch and as a controller for the user to call abort on.

sanderpick · 2023-08-01T00:07:40Z

 ): Promise<WaitableTransactionReceipt> {
  if (config.autoWait ?? false) {
-    const waited = await receipt.wait();
+    const waited = await receipt.wait(controller);


This is the actual fix for the polling handling. wait was being called without any options / controller.

sanderpick · 2023-08-01T00:08:34Z

  config: ReadConfig,
  statement: string,
-  opts: Signal = {}
+  signal?: Signal


Renamed. As mentioned above for controller, this isn't really an "options" object.

We left it as options in case we wanted to add additional opts to it at a later date.

I see. So you would have unioned Signal with some other type in the future? Are you suggesting I change it back?

Yes we could have, but naw it is probably fine. Worst case is that we have to change the parameter name in the future. But since it is already an object, it'll be a pretty safe change.

sanderpick · 2023-08-01T00:09:26Z

  async first<T = S, K extends keyof T = keyof T>(
    colName?: K,
-    opts: SignalAndInterval = {}
+    controller?: PollingController


Is there any reason this was not optional and was instead an empty object by default. I made this change in a bunch of places.

Providing a default value makes it effectively optional. If you look at the typescript signature, it includes the ?. That way, you don't have to double check that a) opts is provided, and then b) if a given property on opts is provided. With chained ? this isn't as much of an issue, but it seemed to lead to cleaner code.

right, but if you look at the type, it doesn't have optional values anymore. you only ever need to do things like controller?.<param>.

Yeh that's true, with those chained ? it is pretty clean. Ok!

sanderpick · 2023-08-01T00:10:35Z

+          await this.#waitExec(
+            {
+              type,
+              sql,
+              tables,
+            },
+            controller
+          );


The old opts var was being injected into the statement object, never to be seen again. Pulling the controller out into its own var makes it easier to see what's going on.

sanderpick · 2023-08-01T14:42:51Z

Related, after poking around in here, wouldn't it be cleaner to use a traditional options object on the statement api that included the colName for filtering and the "polling controller", ie, something like ef8f74f ?

Although, maybe this is part of the codebase that has to conform to D1?

carsonfarmer · 2023-08-01T15:56:04Z

Yeh those are D1 APIs, please don't change without ensuring they are backward compatible with 3rd party libs.

sanderpick · 2023-08-01T16:06:34Z

Yeh those are D1 APIs, please don't change without ensuring they are backward compatible with 3rd party libs.

K, do you have a ref to the API that you're conforming to here? I see a mention of D1 on the Database object, but nothing here.

carsonfarmer · 2023-08-01T16:28:53Z

Yeh, we should actually have a reference to its types. IIRC there was an issue a while back where we couldn't just do "implements" or "extends" because the D1 types didn't actually match the response you would get in practice. But they come from here: https://github.com/cloudflare/workerd/blob/main/types/defines/d1.d.ts and we can probably use the types from @cloudflare/worker-types now? Note that differences to these types are fine as long as they are backward compatible.

carsonfarmer · 2023-08-01T16:30:34Z

Ooop, just noticed that all doesn't support colName anymore! So we could probably remove that to simplify the API?

sanderpick · 2023-08-01T16:34:27Z

Cool, thanks! I see colName here: https://github.com/cloudflare/workerd/blob/main/types/defines/d1.d.ts#L26. Is that what you're looking at? Maybe it's just on first now in their API? btw, I don't have a strong pref here, it just felt weird to type undefined in the all method to be able to use the custom timeout.

joewagner · 2023-08-01T16:46:40Z

@sanderpick This looks like great improvements in general. I agree the current API is very confusing, and turns out broken.
I notice you're pointing this PR at main and I was hoping to stop using this repo this week. However, in order to do that we need to get all the repos aligned on linting, prettier, and tests. I don't think there will be much issue here since the monorepo standards are mostly all being taken from this repo, but can you point this PR at the joe/lint branch?

sanderpick · 2023-08-01T18:08:39Z

@joewagner Absolutely. Just wanted to get a first take on this. I'll try to get that done today.

sanderpick force-pushed the sander/fix-polling-opts branch 3 times, most recently from f2191f3 to 55cf81d Compare July 31, 2023 22:27

fix abort controller handling

b59a0db

Signed-off-by: Sander Pick <sanderpick@gmail.com>

sanderpick force-pushed the sander/fix-polling-opts branch from 55cf81d to b59a0db Compare July 31, 2023 22:56

sanderpick commented Aug 1, 2023

View reviewed changes

dtbuchholz mentioned this pull request Oct 19, 2023

fix abort controller handling tablelandnetwork/tableland-js#36

Merged

Conversation

sanderpick commented Jul 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details

How it was tested

Checklist:

Uh oh!

sanderpick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanderpick commented Aug 1, 2023

Uh oh!

carsonfarmer commented Aug 1, 2023

Uh oh!

sanderpick commented Aug 1, 2023

Uh oh!

carsonfarmer commented Aug 1, 2023

Uh oh!

carsonfarmer commented Aug 1, 2023

Uh oh!

sanderpick commented Aug 1, 2023

Uh oh!

joewagner commented Aug 1, 2023

Uh oh!

sanderpick commented Aug 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sanderpick commented Jul 29, 2023 •

edited

Loading