GV14982 / async-airtable

A lightweight npm package to handle working with the Airtable API.
https://www.npmjs.com/package/asyncairtable
MIT License
54 stars 5 forks source link

Handle Timeout or server crash during fetching of multiple pages of records #59

Closed MoralCode closed 3 years ago

MoralCode commented 3 years ago

this is the beginnings of a fix for #58. So far I have only restructured the error checking to add an else block where this issue can be handled, but haven't yet gotten to figuring out how to actually start fetching over from the beginning as suggested by the Airtable docs

MoralCode commented 3 years ago

Thinking maybe a good way to do this might be to throw a specific kind of error and catch it at the point where select is called and implement the retry logic there.

GV14982 commented 3 years ago

Hey there, I'm going to close this for now, as it seems to have caught up my github actions.

MoralCode commented 3 years ago

Hey there, I'm going to close this for now, as it seems to have caught up my github actions.

what do you mean "caught up my github actions"? like was this PR breaking the existing workflows?

GV14982 commented 3 years ago

Hey there, I'm going to close this for now, as it seems to have caught up my github actions.

what do you mean "caught up my github actions"? like was this PR breaking the existing workflows?

Ooops sorry for the late reply.

Yeah it had got the github actions stuck in a state where it wouldn't allow me to restart these, and it wouldn't start any new ones.

Let me know if you end up doing any more work on this and I'll be happy to re-open.

MoralCode commented 3 years ago

That seems weird. Do you think i would be able to recreate the problem in a fork of this repo using the same github actions? Are they Github actions that were added to the main branch since i made this PR?

I can probably play with it to see if i can get it working (maybe rebasing to the lastest main branch would help?), but other than that, as far as i remember this fix was "done". Is there anything you feel needs work on before it can get merged?

GV14982 commented 3 years ago

Ah I didn't realize it was done already. So two things:

  1. Maybe try rebasing from develop
  2. Make sure to change the target for this PR to develop. Our branching strategy is: branch -> develop -> main

The develop branch equates to the @next tag in NPM.

MoralCode commented 3 years ago

Turns out that there was still a TODO in my code since i wasnt sure how to go about restarting the whole fetch operation (more on this later). Also in trying to rebase it seems like the code has changed significantly.

In looking at what changed, it seems like my first commit that checks for HTTP status 429 when determining whether to retry a request has already been incorporated into rateLimitHandler.ts around line 26 with the code:

if (res.status === 429) {
            return retryRateLimit(
                ...
            )
        }

given this new structure where it seems like the retry is effectively built into async, it seems like all thats needed to handle a 422 LIST_RECORDS_ITERATOR_NOT_AVAILABLE is to add a check for that 422 status code in the catch blocks of the functions within asyncAirtable.ts (such as find) that deal with fetching records. From there it seems like an appropriate course of action based on the documentation for this error code is to restart the fetch operation from the beginning (this was my TODO comment that i wasnt sure how to actually implement).

Overall though, this bugfix is really only useful in cases where the airtable server may be unreliable (which seemed to be fairly often when there were tons of projects using it to make COVID vaccine location databases). Essentially my understanding is that if the airtable server goes down in the middle of a find request involving many paginated API calls to airtable, asyncAirtable will correctly begin retrying the requests, but when the server comes back, it may have some internal state that got reset by the downtime and thus it cannot find the "list records iterator" (whatever that is) when it receives a request for paginated airtable records that, from its perspective, is starting to look for records from the middle of the list.

I have no idea if this is how airtable actually works or if handling this situation is something that would be useful for the reliability of asyncAirtable, but id be more than happy to help finish this feature if you could help me understand what the best way to restart the fetching of paginated records from the beginning is.

GV14982 commented 3 years ago

Hm, let me do some thinking on this a bit. We may want to introduce a cancel method into the base class that allows us to cancel transactions that are in progress. This is pretty trivial from an HTTP standpoint, as there are methods built into fetch (and by extension node-fetch) to allow for this. We will just want to think of a good way to keep track of currently in progress transactions. This would allow us to cancel the current transaction and create a brand new one to handle the 422 error code.