dmcquay / node-apac

node-apac - Node.js client for the Amazon Product Advertising API, including support of Request Signatures
MIT License
499 stars 111 forks source link

Throttled request with promise #73

Closed Raidus closed 7 years ago

Raidus commented 7 years ago

I'm having problems handling an arbitrary number requests. It seems that the throttling doesn't work with promises:

var credentials = require('./credentials');
var Promise = require("bluebird");

var util = require('util'),
    OperationHelper = require('apac').OperationHelper;

var opHelper = new OperationHelper({
    awsId: credentials.aws['awsId'],
    awsSecret: credentials.aws['awsSecret'],
    assocId: credentials.aws['assocId'],
    locale: 'DE',
    maxRequestsPerSecond: 1
});

var ASINs = [
    'B00CYG1AMK',
    'B00CYG17D2',
    'B00CYG1AO8',
    'B00CYG1AMK',
    'B00CYG17D2',
    'B00CYG1AO8',
    'B00CYG1AMK',
    'B00CYG17D2',
    'B00CYG1AO8',
    'B00CYG1AMK',
    'B00CYG17D2',
    'B00CYG1AO8',
    'B00CYG1AMK',
    'B00CYG17D2',
    'B00CYG1AO8',
    'B00CYG1AMK',
    'B00CYG17D2',
    'B00CYG1AO8',
    'B00CYG1AMK',
    'B00CYG17D2',
    'B00CYG1AO8',
    'B00CYG1AMK',
    'B00CYG17D2',
    'B00CYG1AO8'
];

var get_parent_asins = function (asin) {
  return Promise.map(asin, function(asin) {
    return opHelper.execute('ItemLookup',{
      IdType: 'ASIN',
      ItemId: asin,
      ResponseGroup: 'Large',
      MerchantId: 'All',
      Condition: 'New',
    }).then((response) => {
        var result = response.result;
        return result;
    })
  })
}

get_parent_asins(ASINs).then(function(result) {
  result.forEach(function(each) {
    console.log(each.ItemLookupResponse.Items.Item.ParentASIN);
  })
})

I'll get the first 10-15 results from the requests but then:

Unhandled rejection TypeError: Cannot read property 'Items' of undefined

Is there a workaround for this problem?

Raidus commented 7 years ago

I've tested the mapping with a database insert. The maxRequestsPerSecond (0.5 in this case) is not always fulfilled (see below).

Executing (default): SHOW INDEX FROM test Executing (default): INSERT INTO test (id_lh,asin,created_at,updated_at) VALUES (DEFAULT,'B00CYG1AMK','2016-09-28 10:12:34','2016-09-28 10:12:34'); Executing (default): CREATE TABLE IF NOT EXISTS test (id_lh INTEGER auto_increment , asin VARCHAR(255), created_at DATETIME NOT NULL, updated_at DATETIME NOT NULL, PRIMARY KEY (id_lh)) ENGINE=InnoDB;

Executing (default): SHOW INDEX FROM test Executing (default): INSERT INTO test (id_lh,asin,created_at,updated_at) VALUES (DEFAULT,'B00CYG17D2','2016-09-28 10:12:34','2016-09-28 10:12:34'); Executing (default): CREATE TABLE IF NOT EXISTS test (id_lh INTEGER auto_increment , asin VARCHAR(255), created_at DATETIME NOT NULL, updated_at DATETIME NOT NULL, PRIMARY KEY (id_lh)) ENGINE=InnoDB;

Executing (default): SHOW INDEX FROM test Executing (default): INSERT INTO test (id_lh,asin,created_at,updated_at) VALUES (DEFAULT,'B00CYG1AO8','2016-09-28 10:12:37','2016-09-28 10:12:37'); Executing (default): CREATE TABLE IF NOT EXISTS test (id_lh INTEGER auto_increment , asin VARCHAR(255), created_at DATETIME NOT NULL, updated_at DATETIME NOT NULL, PRIMARY KEY (id_lh)) ENGINE=InnoDB;

Executing (default): SHOW INDEX FROM test Executing (default): INSERT INTO test (id_lh,asin,created_at,updated_at) VALUES (DEFAULT,'B00CYG1AMK','2016-09-28 10:12:39','2016-09-28 10:12:39'); Executing (default): CREATE TABLE IF NOT EXISTS test (id_lh INTEGER auto_increment , asin VARCHAR(255), created_at DATETIME NOT NULL, updated_at DATETIME NOT NULL, PRIMARY KEY (id_lh)) ENGINE=InnoDB;

Executing (default): SHOW INDEX FROM test Executing (default): INSERT INTO test (id_lh,asin,created_at,updated_at) VALUES (DEFAULT,'B00CYG17D2','2016-09-28 10:12:41','2016-09-28 10:12:41');

Test script:

'use strict';

var Sequelize = require('sequelize');
var Promise = require("bluebird");
var credentials = require('./credentials');

var Sequelize = require('sequelize');
var db = new Sequelize('....',{
  //logging: false
});

var DB_test = db.define('test', {
    id_lh: {
        type: Sequelize.INTEGER,
        primaryKey: true,
        autoIncrement: true
    },
    asin: {
        type: Sequelize.STRING
    }
}, {
    createdAt: 'created_at',
    updatedAt: 'updated_at',
    underscoredAll: true,
    freezeTableName: true
})

var util = require('util'),
    OperationHelper = require('apac').OperationHelper;

var opHelper = new OperationHelper({
    awsId: credentials.aws['awsId'],
    awsSecret: credentials.aws['awsSecret'],
    assocId: credentials.aws['assocId'],
    locale: 'DE',
    maxRequestsPerSecond: 0.5
});

var ASINs = [
    'B00CYG1AMK',
    'B00CYG17D2',
    'B00CYG1AO8',
    'B00CYG1AMK',
    'B00CYG17D2',
    'B00CYG1AO8',
    'B00CYG1AMK',
    'B00CYG17D2',
    'B00CYG1AO8',
    'B00CYG1AMK',
    'B00CYG17D2',
    'B00CYG1AO8',
    'B00CYG1AMK',
    'B00CYG17D2',
    'B00CYG1AO8',
    'B00CYG1AMK',
    'B00CYG17D2',
    'B00CYG1AO8',
    'B00CYG1AMK',
    'B00CYG17D2',
    'B00CYG1AO8',
    'B00CYG1AMK',
    'B00CYG17D2',
    'B00CYG1AO8'
];

var get_parent_asins = function (asin) {
  return Promise.map(asin, function(asin) {
    return opHelper.execute('ItemLookup',{
      IdType: 'ASIN',
      ItemId: asin,
      ResponseGroup: 'Large',
      MerchantId: 'All',
      Condition: 'New',
    }).then(function () {
      DB_test.sync().then(function() {
          DB_test.create({asin: asin})
        }).then((response) => {
          return response;
      })
    })
  })
}

get_parent_asins(ASINs).then(function(result) {
  result.forEach(function(each) {
    console.log(each);
  })
})
sumpton commented 7 years ago

Thank you for this script.

I had problems with the Throttle as well. I was lazy and changed it to

            const delay = this._nextAvailableRequestMillis - nowMillis;
            // console.log('throttle delay', delay)
           setTimeout(function (text) {
                // console.log('delayed', text);
                return cb()
            }, delay, delay)
            this._nextAvailableRequestMillis += this._timeBetweenRequestsInMilliSeconds
sumpton commented 7 years ago

Sorry, I missed this update

https://github.com/dmcquay/node-apac/commit/2689121bcfade56a61ab5dc3b9105b768ed81bb2

The commit comments are

fix throttle mechanism there were two problems: 1) In the throttle case, the callback was being returned, not executed 2) In the case when throttle was not needed, it was neglecting to wrap the result in a Promise.resolve which wasn't causing any problems directly, but it is bad practice to mix return types.

dmcquay commented 7 years ago

Sorry just catching up here. It looks to me like you found a bug, but the bug was already fixed in 2689121? Therefore this can be closed, right?

sumpton commented 7 years ago

Yes, please, and thanks again for your work.

The throttling works perfectly. I have a debug statement for the length of the delay and am iterating through thousands of products, needing to check mongo if we have a product before a new request. I found that at times there was not a delay, so was not optimized, but async.eachLimit of 2 has delays between a few hundred milliseconds to about 1500. This means we can squeeze in an ad hoc request and it gets slipped in within about 2 seconds and the iteration carries on.

@Raidus may still have problems, but should check the version. I installed the library a few months ago, but didn't use it until recently.

dmcquay commented 7 years ago

Thanks @sumpton @Raidus If you are still having issues, please re-open. I'm glad to help.

nilsi commented 7 years ago

Im doing something like this to fetch products from different amazon stores.

        const opHelperDE = new OperationHelper({
            awsId:     'id',
            awsSecret: 'secret',
            assocId:   Meteor.settings.amazon_partner_id,
            locale:    'DE',
            maxRequestsPerSecond: 1
        });

        const opHelperFR = new OperationHelper({
            awsId:     'id',
            awsSecret: 'secret',
            assocId:   Meteor.settings.amazon_partner_id,
            locale:    'FR',
            maxRequestsPerSecond: 1
        });

        const opHelperUK = new OperationHelper({
            awsId:     'id',
            awsSecret: 'secret',
            assocId:   Meteor.settings.amazon_partner_id,
            locale:    'UK',
            maxRequestsPerSecond: 1
        });

        const german = await searchStore(opHelperDE, searchString);
        const france = await searchStore(opHelperFR, searchString);
        const uk = await searchStore(opHelperUK, searchString);

Im still getting the throttle error from Amazon API. Is this because I'm using different operation helpers? Will put a timeout around my searchStore call and see if it helps.

dmcquay commented 7 years ago

That sounds likely. The throttling should be per operation helper. On Thu, Mar 16, 2017 at 11:05 PM Nicklas Nilsson notifications@github.com wrote:

I doing something like this to fetch products from different amazon stores.

    const opHelperDE = new OperationHelper({
        awsId:     'id',
        awsSecret: 'secret',
        assocId:   Meteor.settings.amazon_partner_id,
        locale:    'DE',
        maxRequestsPerSecond: 1
    });

    const opHelperFR = new OperationHelper({
        awsId:     'id',
        awsSecret: 'secret',
        assocId:   Meteor.settings.amazon_partner_id,
        locale:    'FR',
        maxRequestsPerSecond: 1
    });

    const opHelperUK = new OperationHelper({
        awsId:     'id',
        awsSecret: 'secret',
        assocId:   Meteor.settings.amazon_partner_id,
        locale:    'UK',
        maxRequestsPerSecond: 1
    });

    const german = await searchStore(opHelperDE, searchString);
    const france = await searchStore(opHelperFR, searchString);
    const uk = await searchStore(opHelperUK, searchString);

Im still getting the throttle error from Amazon API. Is this because I'm using different operation helpers? Will put a timeout around my searchStore callbacks and see if it helps.

— You are receiving this because you modified the open/close state.

Reply to this email directly, view it on GitHub https://github.com/dmcquay/node-apac/issues/73#issuecomment-287267934, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHIcbRBhDpWOcdcSgw80xGd7HVTu9S4ks5rmhSrgaJpZM4KH203 .