google-api-nodejs-client icon indicating copy to clipboard operation
google-api-nodejs-client copied to clipboard

How to Catch Rate Limit errors?

Open Apollon77 opened this issue 5 years ago • 10 comments

Hi,

we use your great nodejs library to integrate with Google Drive. We wrap your library basically in https://github.com/simatec/ioBroker.backitup/blob/master/lib/googleDriveLib.js

Now we have some users that alw<ys get Rate Limiting errors and in fact these are ok, but something seems to be wrong because they are thrown as unhandled promise rejections in the end and are not catchable. I also did a dive through your code and basically yes it should work, but we still get these reports and also our Sentry instance shows them:

One case: https://sentry.iobroker.net/share/issue/cdcb2ff3d08042f7829bb551e73a7043/

Error: User Rate Limit Exceeded. Rate of requests for user exceed configured project quota. You may consider re-evaluating expected per-user traffic to the API and adjust project quota limits accordingly. You may monitor aggregate quota usage and adjust lim...
  File "/opt/iobroker/node_modules/gaxios/src/gaxios.ts", line 117, col 15, in Gaxios._request
  ?, in runMicrotasks
  File "internal/process/task_queues.js", line 97, col 5, in processTicksAndRejections
  File "/opt/iobroker/node_modules/google-auth-library/build/src/auth/oauth2client.js", line 343, col 18, in OAuth2Client.requestAsync
    r2 = await this.transporter.request(opts);

Second case: https://sentry.iobroker.net/share/issue/a325278331b14b81ac79337e65a6be86/

Error: Rate Limit Exceeded
  File "/opt/iobroker/node_modules/iobroker.backitup/node_modules/gaxios/src/gaxios.ts", line 117, col 15, in Gaxios._request
  File "internal/process/next_tick.js", line 68, col 7, in process._tickCallback

We use the most current version of googleaps here: 59.0.0

In fact yes we need to optimize the timing when (as it seems) we delete many files (https://github.com/simatec/ioBroker.backitup/blob/master/lib/scripts/75-googledrive.js#L43 ...) but in fact the error should come to the callback as expected and should not be uncatchable thrown ...

Apollon77 avatar Sep 04 '20 05:09 Apollon77

Greetings! I took a look at your code, and it has a few issues that could be leading to the problem here. Your code is mixing promises and call backs in some confusing ways. This code:

 deleteFile(folderOrFileId, fileName) {
        return new Promise((resolve, reject) => {
            if (folderOrFileId && !fileName) {
                this._getDrive()
                    .then(drive => drive.files.delete({fileId: folderOrFileId}, err => {
                        if (err) {
                            // Handle error
                            reject(err);
                        } else {
                            resolve();
                        }
                    }));
            } else {
                this.getFileOrFolderId(fileName, folderOrFileId)
                    .then(fileId => {
                        this._getDrive()
                            .then(drive => drive.files.delete({fileId}, err => {
                                if (err) {
                                    // Handle error
                                    reject(err);
                                } else {
                                    resolve();
                                }
                            }));
                    });
            }
        });

Could be simplified to:

async function deleteFile(folderOrFileId, fileName) {
  const drive = await this._getDrive()
  if (folderOrFileId && !fileName) {
    await drive.files.delete({fileId: folderOrFileId});
  } else {
    const fileId = await this.getFileOrFolderId(fileName, folderOrFileId);
    await drive.files.delete({fileId});
  }
}

When you call one of these methods as a promise, whether with .then chaining or async functions, it's going to throw if you hit an exception. If you want to handle specific cases, you can always wrap this in a try/catch, and then look for err.response.status, which should tell you what caused this specific problem.

JustinBeckwith avatar Sep 04 '20 14:09 JustinBeckwith

I know that the code is "suboptimal" ;-) We will try your proposal (and yes look much cleaner) and we will get results very fast I think

Apollon77 avatar Sep 04 '20 22:09 Apollon77

@JustinBeckwith As it was my fear did not really changed the situation also with your code now (v1.6.5 of our software ...). So it seems that there is a "hole" in the "bubble up exceptions in async/await" in the google libs :-(

Apollon77 avatar Sep 08 '20 07:09 Apollon77

In the code I posted above, I'd expect you need to wrap this in a try catch:

async function deleteFile(folderOrFileId, fileName) {
  const drive = await this._getDrive()
  if (folderOrFileId && !fileName) {
    await drive.files.delete({fileId: folderOrFileId});
  } else {
    const fileId = await this.getFileOrFolderId(fileName, folderOrFileId);
    await drive.files.delete({fileId});
  }
}
try {
  await deleteFile();
} catch (e) {
  console.log(e.response);
}

Would something like that work for you?

JustinBeckwith avatar Sep 08 '20 11:09 JustinBeckwith

We did the try/catch inside the method ... https://github.com/simatec/ioBroker.backitup/blob/master/lib/googleDriveLib.js#L203 so slightly different but "same". This is that way inthe v1.6.6 where we also get these exceptions exactly "the same" as before

Apollon77 avatar Sep 13 '20 15:09 Apollon77

Greetings, we're closing this. Looks like the issue got resolved. Please let us know if the issue needs to be reopened.

fhinkel avatar Dec 07 '20 10:12 fhinkel

Please reopen it, we still have this problem about uncatchable error cases

Apollon77 avatar Dec 07 '20 11:12 Apollon77

Also running into a similar problem, but in our case it doesn't seem to throw any errors. I've created a minimal script to reproduce the issue. Have created a separate issue as not sure if related.

sweetanalytics avatar Dec 29 '20 16:12 sweetanalytics

@sweetanalytics could you please link the new issue to here?

Apollon77 avatar Dec 29 '20 16:12 Apollon77

@Apollon77 https://github.com/googleapis/google-api-nodejs-client/issues/2495

sweetanalytics avatar Dec 29 '20 16:12 sweetanalytics

Hi @Apollon77, I believe you're getting unhandled rejections due to the mixing of chaining and async functions. I've reproduced your code with a similar example, see below:

const {google} = require('googleapis');

const drive = google.drive('v3');

async function runSample() {
  // Obtain user credentials to use for the request
  const auth = new google.auth.GoogleAuth({
    keyFilename: 'service-account-credentials',
      // Scopes can be specified either as an array or as a single, space-delimited string.
    scopes: ['https://www.googleapis.com/auth/drive',
    'https://www.googleapis.com/auth/drive.file',
    'https://www.googleapis.com/auth/drive.appdata']
  });
  const authClient = await auth.getClient();
  google.options({auth: authClient});

  try {
    await drive.files.get({fileId: '1jkXTckKOBtAajJmN2C9eWFlabaZZFmpc9bIaRp6JySc'});
    console.log('success')
} catch (e) {
    console.log('CAUGHT!')
    console.log(e)
}
}

for (let x=0; x< 20001; x++) {
    runSample();
}

This is my output:

...
success
success
success
success
success
success
success
success
success
success
success
success
success
success
success
success
CAUGHT!
GaxiosError: User Rate Limit Exceeded. Rate of requests for user exceed configured project quota. You may consider re-evaluating expected per-user traffic to the API and adjust project quota limits accordingly. You may monitor aggregate quota usage and adjust limits in the API Console
}

I believe that if you were to just call deleteFiles without chaining it to other promises (and by faking any Gaxios error), you would be able to catch it as well.

sofisl avatar Jan 14 '23 00:01 sofisl

I'm going to close for now, but feel free to reopen if you don't get the same response. Thanks!

sofisl avatar Jan 18 '23 00:01 sofisl

Thank you for looking into it, we will check code if we still have such mixed cases. thank you!

Apollon77 avatar Jan 18 '23 09:01 Apollon77