0

Background

The following program tracks events. For simplicity, each event has a bfId, baHome.id, baAway.id and it generates a performance object where each key is a period of periods and contains home and away, each containing scored and conceded fields (please see structure below)

Problem

I get a variable leak while instances of updatePerformance() are running in parallel. bfId changes while waiting for getPerformance() to resolve. I'm assuming it is a scope issue and I'm passing a reference which is changing somewhere, but it shouldn't. Any ideas on how to tackle this?

Failed attempts

I've tried passing a queryId (random number) to getPerformance() and returning it back to updatePerformance(). No errors there, but bfId changes within updatePerformance() after awaiting and it adds it to the wrong event. Why is this happening?

Global object format

// trackedEvents generates an object where keys are bfIds, in the following format:
{
    '932481': {
        baHome: { id: 1234 },
        baAway: { id: 4911 },
        performance: {
            '5': { home: { scored: 9, conceded: 4 }, away: { scored: 1, conceded: 3 } },
            '10': { home: { scored: 12, conceded: 7 }, away: { scored: 3, conceded: 9 } },
            // ... '15', '20'
        }
    }
}

Code

Here's some simplified code:

const periods = [ 5, 10, 15, 20 ];
let trackedEvents = {};

;(async () => {
    const bfIds = await getBfIds();
    for(bfId of bfIds)
        addEvent(bfId);
})();


async function addEvent(bfId) {
    // Event is new, add to global object
    trackedEvents[bfId] = {
        baHome: { id: getHomeId(bfId) },
        baAway: { id: getAwayId(bfId) }
    }

    await updatePerformance(bfId);
}

async function updatePerformance(bfId) {
    const event = trackedEvents[bfId];

    if(!event.performance)
        trackedEvents[bfId].performance = {};

    // Queueing performance periods
    for(period of periods)
        if(!event.performance[period])
            performanceQueue.push(getPerformance(event.baHome.id, event.baAway.id, period));

    // Wait for performance periods to resolve
    const performance = await Promise.all(performanceQueue);

    // Add performance is specified format // ATTENTION: bfId changes after await
    performance.forEach((result) => {
        trackedEvents[bfId].performance[result.period] = result;
    });
}

// Returns all performance for period
async function getPerformance(baHomeId, baAwayId, period) {
    const [ home, away ] = await Promise.all([
        getLastStats(baHomeId, period), // Get home team performance
        getLastStats(baAwayId, period) // Get away team performance
    ]);

    return { home, away, period };
}

// Returns performance for home or away team
async function getLastStats(baId, period) {
    // Fetch last "period" games from db where team played
    const lastGames = await db.collection('events').find({
        $or: [{ "baHome.id": baId }, { "baAway.id": baId }],
        "score.home": { $exists: true }
    }).sort({ openDate: -1 }).limit(period).toArray();

    // Add scored and conceded goals
    let scored = 0, conceded = 0;
    for(game of lastGames) {
        const [ homeScore, awayScore ] = [ parseInt(game.score.home), parseInt(game.score.away) ];

        // Team playing as home team
        if(baId == game.baHome.id) {
            scored += homeScore;
            conceded += awayScore;
        }

        // Team playing as away team
        else {
            scored += awayScore;
            conceded += homeScore;
        }
    }

    return { scored, conceded };
}
Esteban Ortega
  • 155
  • 1
  • 8
  • 2
    `for(const bfId of bfIds)`, not `for(bfId of bfIds)`. Same for all the other `for`–`of` loops. You need to declare your variables so they become appropriately scoped and don’t end up as global properties. Use tools like [JSHint](//jshint.com/) to find problems with your code immediately and [use strict](//developer.mozilla.org/docs/Web/JavaScript/Reference/Strict_mode). – Sebastian Simon Jan 29 '22 at 04:08
  • @SebastianSimon You're the man. This is the answer I was looking for. Feel free to add it as an answer and I'll mark it as solved. Thank you so much for your help! – Esteban Ortega Jan 29 '22 at 04:14
  • @SebastianSimon Out of curiosity, why would a for loop inside a function be sharing memory with another instance? Isn't everything inside that function its own scope? Would the same happen with 'let' declarations? – Esteban Ortega Jan 29 '22 at 04:41
  • It’s not about “sharing memory”. `x = 1;` outside of strict mode implicitly creates a global property. This bypasses any scoping rules, because `x` is not a variable, but a property on the global object. `for(x of iterable)` implicitly does `x = iterable[0];` (or something similar) in the first iteration (and the other indexes in the subsequent iterations), so it’s the same problem. `let` scopes the variable to a single iteration, so does `const` (but makes the binding immutable in the iteration); `var` scopes it to the entire enclosing function. – Sebastian Simon Jan 29 '22 at 05:05
  • `why would a for loop inside a function be sharing memory with another instance` -- Because you are using a global variable. That's what global variables do. All variables declared with `var`, `let` and `const` are not global variables. However your variable is declared with none of them so it is a global variable. – slebetman Jan 29 '22 at 05:09
  • @slebetman It’s technically a property, not a variable. Most of the time, a variable and a global property can be used interchangeably, but it’s important to be aware that there is a difference. – Sebastian Simon Jan 29 '22 at 05:13
  • Nope. It's a variable. Not a property. You may be confused about the fact that javascript mirrors variables in global scope as members of the global object but that is just implementation detail. The ECMAScript spec itself talks about global variables as **variables**. – slebetman Jan 29 '22 at 06:42
  • @slebetman Sorry, no. Outside strict mode, `x = 1;` serves as a shorthand for `globalThis.x = 1;`, just like `parseFloat("123");` can serve as a shorthand for `globalThis.parseFloat("123");`. One effect of this is that it’s possible to actually delete properties as `delete x;` (again, outside strict mode); it’s not possible to “delete” variables this way. The spec never mentions “global variable”, it only mentions “global var”, but never in reference to assignments without the `var` keyword. – Sebastian Simon Jan 30 '22 at 08:01
  • The [strict mode](//tc39.es/ecma262/#sec-strict-mode-of-ecmascript) section says that, specifically in strict mode, _“Assignment to an undeclared identifier […] does not create a property in the global object.”_ — _Property_, not variable. The fact that `var x = 1;` in global scope _also_ puts `x` as a property on `globalThis` is explicitly specified in [CreateGlobalVarBinding](//tc39.es/ecma262/#sec-createglobalvarbinding) (via the CreateMutableBinding method of the [[ObjectRecord]] Object Environment Record property, which corresponds to the global object), not an implementation detail. – Sebastian Simon Jan 30 '22 at 08:02
  • Statements like “{ `var`, `const`, `let` } _Identifier_ `=` _AssignmentExpression_ `;`” put the binding in the appropriate _VariableEnvironment_ or _LexicalEnvironment_. `x = 1;` by itself doesn’t. Any assignment performs [PutValue](//tc39.es/ecma262/#sec-putvalue), which in turn uses IsUnresolvableReference on the left-hand side. For a script which only contains `x = 1;`, this abstract operation will return `true`, so PutValue will proceed with Set(_globalObj_, _V_.[[ReferencedName]], _W_, `false`); Set is an abstract operation to set _properties_, not bind variables. – Sebastian Simon Jan 30 '22 at 08:03
  • For `var`, `let`, and `const` declarations in global scope, bindings are created at [ScriptEvaluation](//tc39.es/ecma262/#sec-globaldeclarationinstantiation). `CreateGlobalVarBinding`, `CreateMutableBinding`, or `CreateImmutableBinding` is used on the global environment. [FunctionDeclarationInstantiation](//tc39.es/ecma262/#sec-functiondeclarationinstantiation) does something similar. A variable declaration with `const` or `let` performs [InitializeReferencedBinding](//tc39.es/ecma262/#sec-initializereferencedbinding) which creates the binding in the appropriate Environment Records. – Sebastian Simon Jan 30 '22 at 08:04
  • @SebastianSimon Again you are confusing the fact that globalThis is how the interpreter implements global scope with the thing itself. The `globalthis.x` is just a mechanism that mirrors all global variables. The thing itself is still a global variable and participates in all the mechanics that affect variables. – slebetman Jan 31 '22 at 04:39
  • @SebastianSimon This argument is silly anyway. You are insisting on telling the OP that the reason why his variable keeps getting overwritten is because his variable is a property of `globalThis`. And then you need to explain to him that all properties of `globalThis` are accessible and shared globally in the runtime environment -- **YOU HAVE JUST DEFINED WHAT GLOBAL VARIABLES MEAN**. This is like telling someone to start his car rolling his output gear ratio needs to have enough torque to overcome stiction and inertia while I'm just telling him to start with first gear. – slebetman Jan 31 '22 at 04:42
  • @slebetman I didn’t explain the difference between properties and variables _to the OP_. _“It’s technically a property, not a variable”_ was a minor nitpick in response to _your_ comment. In the context of the original question, of course this technical discussion about semantics is useless. – Sebastian Simon Jan 31 '22 at 08:59

0 Answers0