0

I have a programming problem, that I don't know how to solve. And while I have provided a sample of my code, I am interested in a conceptual answer on how to resolve this problem.

On a tradeOffers event, I call a function to get a list of offers and process them one by one using async.forEachOfSeries from Async.js module. However, if a new offer comes in while pending offers are still processing then the same offer can be processed multiple times and also return an offer unavailable error because it was already accepted. This is a problem because the new offer is effectively not evaluated.

Is it possible to solve this with simple callback or true / false flag, or would I need to implement a queue for the offers? If I need to use a queue, then I would like to avoid using SQL to store the offers. Or do I need a more complex control mechanism to handle new tradeOffers events when processing is already in place?

I am currently using steam-tradeoffers and steam-user module.

To better explain my current situation, here is the code:

    steam.on('tradeOffers', function(count) {
        if (count > 1 && bot_working == true) {
            processoffers();
        }
        });

    function processoffers() {
        if (bot_working) {
            offers.getOffers({
                get_received_offers: 1,
                active_only: 1,
                get_sent_offers: 0,
                get_descriptions: 1,
                time_historical_cutoff: Math.round(Date.now() / 1000),
                language: "en_us"
            }, function(error, body) {
                if(error) return;
                if(body.response.trade_offers_received){
                async.forEachOfSeries(body.response.trade_offers_received, function(offer, key, cboffer) {
                        if (offer.trade_offer_state == 2){
                            //process offer
                        }
                        }, function (err) {
                if (err) console.log(err.message);
                  console.log('Completed all incoming offers.');
                }
            ); //function foreach
        } //if trade tradeoffers
    }); //if getoffers
};
};
  • Although in this particular case I think you can just use promises, why do you want to "avoid SQL" in general? – Ixrec Jan 10 '16 at 18:10
  • No not in general offer is inserted in database, because I'm trying to avoid waiting for query from sql. I just found out about queue in Async.js maybe should I try to use that with concurrency of 1 ? Could that replace async.forEachOfSeries to process offers one by one and to use it to store offers ? – Skanker Jan 10 '16 at 18:34
  • Gotcha. Now, when you say "the same offer can be processed multiple times", are you saying the body.response.trade_offers_received array is getting modified by code outside the processoffers() function? If so, then "make a defensive copy" is probably a better answer than introducing queues and playing with low-level concurrency settings. If you aren't sure, this becomes a debugging question in need of an MVCE. – Ixrec Jan 10 '16 at 18:51
  • Yes that array is returned from offers.getOffers() and changes everytime event tradeOffers is triggered. This code does work, but problem is if that if two events come in the same time, function processoffers() is called twice and the same offers get processed twice. – Skanker Jan 10 '16 at 19:17
1

If the array you're asynchronously processing can be modified by other code, problems like this are bound to happen at some point. The simplest way to prevent this kind of race condition is to make a defensive copy.

The "multiple acceptance" bug you describe makes it sound like this callback is mutating the actual objects in the array, rather than merely changing which objects the array contains, so you'll probably have to use a deep copy rather than a shallow copy for this to work.

Ideally, the defensive copy would be done in the implementation of getOffers() before calling the callback, so that call sites never have to worry about these problems. Personally, I always throw in a defensive copy whenever I implement a method like this, because it preemptively prevents so many subtle bugs in client code. Whether I use a deep or a shallow copy depends on how much control I'm supposed to have over the objects I'm passing to client code.


But sometimes, the array or the objects it contains might be too large for a deep defensive copy to be practical. In that case, you're going to need a more interesting solution. I don't know if this applies to your problem, but for the sake of completeness...

  1. You could try to ensure that tradeOffers never runs while there is an unfinished getOffers() request. As you suggested, the most straightforward way of doing this is to maintain a queue of requests and process them one at a time. But even that approach will require the getOffers() callback to somehow tell you when it's done executing all of its async bits. Relying on client code to always return a promise or always call a special callback when it's really really done is...risky to say the least.
  2. A quick hacky solution would be to stick to fully synchronous code inside the getOffers() callback. However, this relies on the assumption that getOffers() calls your callback synchronously rather than asynchronously, otherwise you still have one tick where a tradeOffers event can sneak in. Maybe you happen to know that getOffers() always calls its callbacks synchronously. But personally, even if I did know that, this sort of subtle timing assumption makes me uneasy because it comes a bit too close to "releasing Zalgo".
  3. You could store the offers in an actual database, since most databases have already solved this problem.

In case it isn't obvious, I'd go with a database when the data is too big to copy. Relying on databases to do heavy lifting tends to prevent a lot of headaches.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.