0
if (Object.keys(globalObject).length != 0) {
  Object.keys(globalObject).forEach(function(key) {
    database.query('SELECT * from accounts WHERE `ID` = ' + key + ' LIMIT 1', function(error, rows, fields) {
      if (rows.length == 0) return;
      console.log('User has  ' + globalObject[key].length + ' items')
      globalObject[key].forEach(function(item) {
        console.log('Item ID is ' + item.id)
      });
      delete globalObject[key]
    })
  })
}

globalObject example (varies constantly depending on connected users and each user items):

globalObject = { "1001":[{id:1},{id:2}], "1002":[{id:2},{id:3}]}

Sometimes I get fatal error, crash:

TypeError: Cannot read property 'length' of undefined

At this line:

console.log('User has  ' + globalObject[key].length + ' items)

Despite pushing different users at any time from different parts of the program, the only place I do delete globalObject[key] is after I am done processing the user.

So how come that a key can not exist (undefined) when I've accessed it through an iterator (forEach) that ensures the key exists and it won't be deleted until the very end?

EDIT:

I think the reason is I call the forEach inside a setInterval (every 200ms), so before the forEach has finished it gets called again, therefore deleting the key. How can I make this more synchronous to avoid being called twice for the same keys in a small timespan?

needitohelp
  • 103
  • 2
  • 7
  • If an object has no keys (an empty object), it's obviously going to give you an error. – frozen Jul 14 '17 at 20:09
  • 1
    A small improvement outside of your immediate problem: start with `let keys = Object.keys(globalObject); if (keys.length > 0) { keys.forEach(...) ...`. – Mike 'Pomax' Kamermans Jul 14 '17 at 20:10
  • While everyone here is on a wild goose chase, I'm voting to close this as off-topic due to not being a [mcve]. You have a program with a global variable, which you admit is being updated between the time this function runs and when each `database.query()` calls back. Without further information, it's impossible to tell you with any certainty what is _actually_ causing the problem, we can only guess. Please [edit] your question to make this problem reproducible with as little code as possible. – Patrick Roberts Jul 14 '17 at 20:13
  • are you replying to emails or something? I deleted that comment right after posting it because I noticed that too late. – Mike 'Pomax' Kamermans Jul 14 '17 at 20:23
  • 1
    You probably want to stick a simple console log in your code to make sure you understand what's going on. In addition to capturing `keys` as its own list, maybe set a `keys.forEach(key => { let data = globalObject[key]; console.log(`processing key ${key} with associated data ${JSON.stringify(data)}`);` so that you see what your code is going to try to work with, and then have a similar log inside your database callback, to see whether there's a data mismatch. It's a good bet your "global object" should never have been global, because other code might be messing with it while your queries run. – Mike 'Pomax' Kamermans Jul 14 '17 at 20:25
  • @Mike'Pomax'Kamermans no, I'm just not in the habit of _constantly_ refreshing my page to check and see if you deleted your erroneous comments while I'm in the middle of replying to them. I assume that's normal behavior no? – Patrick Roberts Jul 14 '17 at 20:28
  • no idea - I just look at my SO notifications, open any notifications in new tabs, check that the comment it's in relation to is still there because plenty of people will delete their comment if they realise it was erroneous, and then if it's still there I'll write a reply. No reason to stay on a page in the mean time. – Mike 'Pomax' Kamermans Jul 14 '17 at 20:32
  • I think the reason is I call the forEach inside a setInterval, so before the forEach has finished it gets called again, therefore deleting the key. – needitohelp Jul 14 '17 at 22:45

1 Answers1

0

The most likely scenario is that you are running more than one of these loops at a time and that creates a potential race condition (because of the asynchronous database call) where one loop deletes a key that the other loop is still working on.

The best fix would be to both avoid ever running two of these at the same time and to also protect against possible race conditions. If you show us the calling code context so we can see how/if it can get called more than once before the other is finished, we can advise on a way to avoid that.

Even without that, you can protect against the race condition like this:

  Object.keys(globalObject).forEach(function(key) {
    database.query('SELECT * from accounts WHERE `ID` = ' + key + ' LIMIT 1', function(error, rows, fields) {
      // if the db says no rows or the key is already gone, then nothing to do
      if (rows.length == 0 || !globalObject[key]) return;
      console.log('User has  ' + globalObject[key].length + ' items')
      globalObject[key].forEach(function(item) {
        console.log('Item ID is ' + item.id)
      });
      delete globalObject[key];
    });
  });

The extra !globalObject[key] check added to the if statement protects against the case where someone else has already removed the key during your database calls.


Incidentally, you don't need this if:

if (Object.keys(globalObject).length != 0) { 

because Object.keys(globalObject).forEach() works fine if the array is empty (it just has nothing to do).


More explanation based on your comments:

The .forEach() loop runs to completion without getting interrupted. But the database operations in the loop are non-blocking. They get started, but the loop doesn't wait for them to finish. So, the .forEach() loop will be done, but the callbacks inside the loop have not yet been called.

So, if you have 10 keys, you will start 10 database operations (and none have completed yet). Now, the .forEach() loop is done. If your setInterval() calls this code again BEFORE all 10 database operations have finished, then a new .forEach() loop will run and start some more database operations, based on keys that are about to get deleted by the first loop (when its database operations finally finish). Now, after the second .forEach() loop has run, the database operations from the first loop start finished and removing keys. Then, after they finish, the database operations from your second .forEach() loop finish and their callbacks are called and they are trying to use keys that have now been deleted by the callbacks inside the first loop.

The loops themselves don't run at the same time. But, after each loop runs, it sets up database callbacks that run at some indeterminate time in the future and those callbacks (as coded by you) were assuming that a certain set of keys had not been deleted. But, if you didn't wait until all the database callbacks from the first loop were done before starting a second loop, then that assumption falls apart as the second loop will be using keys that the first loop's database calls are going to remove when they finally finish. That will mess up the database callbacks that were started by the second loop.

The .forEach() calls have nothing to do with events. They are just loops in Javascript and they run synchronously. The database calls just start asynchronous operations. Their callbacks will be triggered by events sometime in the future. Because those database calls are non-blocking asynchronous, the .forEach() loop finishes and then sometime later the JS interpreter gets events that trigger the database callbacks to get called.

jfriend00
  • 683,504
  • 96
  • 985
  • 979
  • Thank you. I remembered you from https://stackoverflow.com/questions/44728125/is-asynchronicity-a-constant-for-a-functions-or-b-lines-of-code . Hehehe. Events run one at a time. Isn't a foreach loop supposed to be an event? And here I am, with two foreach loops messing with each other. I have found another way to do what I wanted to do, this time without an interval, but I still have the feeling that I don't really know what is asynchronous and what isn't XD. – needitohelp Jul 15 '17 at 06:31
  • @needitohelp - A `forEach` loop is not an event. It has nothing to do with events. It's just a loop in Javascript. Your `forEach` loop runs all its iterations, starting all your database operations and then after the `forEach` loop is done, the database operations finish one by one and call their callbacks. – jfriend00 Jul 15 '17 at 06:34
  • if a second foreach loop can run at the same time while the first one hasn't finished yet, your affirmation in that thread "No two functions can run at the same time." is false, because a forEach loop is a function. – needitohelp Jul 15 '17 at 06:37
  • @needitohelp - I added more to the end of my answer to try to explain the interaction between `.forEach()` loops and your asynchronous database calls. The issue for you is that the database calls are non-blocking so the `.forEach()` loops finish (and no, you don't get two of the actual loops running at the same time), but the database calls inside of them have not yet been called. They are event driven and will run some time later whenever the database finishes its work. – jfriend00 Jul 15 '17 at 06:49
  • Thanks. I understand that each forEach is guaranteed to run one at a time, but the 'content' of the forEach itself is not (depending on what you do inside). Now I know why the async module exists. If I've understood the bigger picture, the reality is that the moment I call an asynchronous function from inside a foreach, I lose the synchronous nature of the foreach. So I guess the secret to become a master of this is to learn to identify fast what is synchronous and what isn't. It's going to take a while XD. – needitohelp Jul 15 '17 at 22:09
  • @needitohelp - Callbacks to async calls are event driven and they can run any time in the future. The fact that you have them in a `.forEach()` ONLY means that the async operation is started in the `.forEach()` loop. The callback will get called some indeterminate time in the future, long after the loop itself has already finished iterating. The fact that the callback is physically located inside the loop means nothing. It's not blocking, it's event driven and will get called sometime later. – jfriend00 Jul 15 '17 at 22:54
  • @needitohelp - Think of the `database.query()` function call like you calling your friend on the phone and leaving a message to call you back some time later. You then go about the rest of your day and sometime later, your friend calls you back. That's how the database function works. You send a message to the database telling it what query you want and some time later, it calls you back and tells you what the results are. Meanwhile, the Javascript interpreter has gone about it's other business and finished the `.forEach()` loop. – jfriend00 Jul 15 '17 at 22:56