Openscope: CID values not properly freed up from _cidNumbersInUse

[cake-pie]

Issue title has been updated to reflect root cause of problem rather than just reported symptoms.
Details given comment further down.

Original bug report follows:

---

Browser: Chrome

Airport: KBOS

Approximate play time before issue:
Playing on-and-off over the course of 24h.
Browser was left open in background when not playing.

Issue Description

I've had this happen three times now:
After playing for a long duration, game halts completely and does not respond to any inputs. Checking the console reveals large (and growing) number of "Uncaught RangeError: Maximum call stack size exceeded" which continue to accumulate unless script execution is halted (F8).

For a rough ballpark, this occurs at a score of >9000 but <10,000.
Most recently happened at 9610 score with console output flooded with:

Example call stack:

value (StripViewController.js:290)
value (StripViewController.js:332)
value (StripViewController.js:332)
value (StripViewController.js:332)
value (StripViewController.js:332)
value (StripViewController.js:332)
... (repeats many, many times) ...
value (StripViewController.js:332)
value (StripViewController.js:332)
value (StripViewController.js:332)
value (StripViewController.js:332)
value (StripViewController.js:332)
value (StripViewController.js:177)
value (StripViewController.js:161)
value (AircraftController.js:426)
value (AppController.js:252)
value (App.js:301)
value (App.js:324)
requestAnimationFrame (async)
value (App.js:321)
requestAnimationFrame (async)
value (App.js:321)
requestAnimationFrame (async)
value (App.js:321)
requestAnimationFrame (async)
value (App.js:321)
requestAnimationFrame (async)
value (App.js:321)
... (repeats many times)

Steps to reproduce

Play for a long time

[cake-pie]

Looks like this recursion is the primary issue (with >3500 levels of recursion in call stack):
https://github.com/openscope/openscope/blob/39b760dbe67abc3836a76eb87ffffd9de399189e/src/assets/scripts/client/aircraft/StripView/StripViewController.js#L328-L338

It could be refactored to use iteration instead, thus avoiding overflow:

do {
    nextCid = _random(1, CID_UPPER_BOUND);
} while (this._cidNumbersInUse.indexOf(nextCid) !== INVALID_INDEX);

However, there may be a deeper underlying issue caused by inability to generate an unused CID even after many many attempts. So:

• It may be better to include a fallback so that after a few attempts to generate a CID by random number generator, the code would just pick the first unused CID instead.
• Noting that CID_UPPER_BOUND = 999 and 10 points of score awarded per successful arrival/departure, correlates well with the issue cropping up at score >9000. It might be a good idea to check if CIDs are being properly freed up.

[n8rzz]

Holy crap, I'm not aware of any users playing(or having the app open) for that long. That's awesome and thank you for the bug report.

Also thank you for the quick analysis, this is very helpful and not at all where I though the issue(s) were coming from. You're probably on the right track with the CID values not properly freeing up. I don't think 24hrs of play time was anywhere close to a use case we thought of, so there may be some weirdness going on simply due to length of play time.

If you'd like to open a PR with a fix for this, we encourage community contributions. Only thing needed there is for @erikquinn to add you to the Openscope group. Feel free to also join our slack channel for easier communication.

[cake-pie]

Have briefly tested and can confirm CIDs not cleaned up.
Stack overflow is just a symptom of the game having run out of CIDs.

Steps to reproduce:

• launch openscope in browser, enable developer tools
• add breakpoints on:
  
  
  
  • _generateCidNumber at StripViewController.js:335
  
  
  • _removeCidFromUse at StripViewController.js:351
  
  

• add watch on this._cidNumbersInUse

• refresh page

• L335 breakpoint triggered during initial load for each departure acft
  
  
  
  • step execution permits observing as CID for each acft added to the in-use list
  
  

• wait for first arrival acft to become controllable -- triggers L335 breakpoint

  • note CID for the arrival acft added to the list

• change airport ( causes clean up of old airport before loading new one )

• L351 breakpoint triggered by clean up of first aircraft
  
  
  
  • note the CID/callsign of the first acft being removed
  
  
  • step out of _removeCidFromUse
    
    
    
    
    
    • notice that the CID is still in the list after exiting the function
    
    
    
  
  
  • resume execution, observe that the strip for that acft is removed
  
  
• L351 breakpoint triggered by clean up of second aircraft
  
  
  
  • notice that the CID of first acft is still in the list
  
  
  • resume execution, observe as subsequent CIDs also fail to be removed
  
  

screenshots

Here is state at the first L351 breakpoint.

• currently preparing to remove the strip for CFS1418 (CID 363)
• note 6 CIDs for departure acft, one for arrival

Here is state at second L351 breakpoint.

• currently preparing to remove the strip for ACA23 (CID 190)
• strip for CFS1418 is gone, but 363 is still in the list of CIDs

[erikquinn]

I still see no possible advantage to creating the CIDs the way we do, and suggest we instead fill an array with the usable values, then shuffled. Create an aircraft, get a cid via .pop(), delete an aircraft, return the cid via .unshift(), and life would be a lot simpler.

Plus, we can run until none remain. When there are none, and one is requested, the method can throw or something. As opposed to generating them on demand and using recursion-- if we're low on CIDs, it might repeat its efforts to find an available one a hundred times.

[erikquinn]

Also, 100% on the thank you to @cake-pie. Very helpful stuff! I'm just in a slight rush to respond, that's why it's blunt above ^

[cake-pie]

suggest we instead fill an array with the usable values

Well, what is a reasonable estimate for maximum number of strips ( = controllable acft ) in the sim at any time, in the typical case? I don't normally exceed two or three dozen under peak conditions in the worst case, though admittedly my experience is from playing mostly KBOS -- and unrealistically at that.

If we generally need fewer than 50 at any time, do you want to keep an array of ~1000 available values?
Current method of randomly generated works fine, I think -- there is a less than 5% chance of a newly generated CID conflicting with one that is already in use and requiring a re-roll.

Just need to fix the underlying problem -- not freeing up after use.

---

And here is the problem:

CIDs are generated and stored in _cidNumbersInUse as numbers.

However, StripViewModel.cid actually stores it as a string (padded with leading zeroes for display):
https://github.com/openscope/openscope/blob/39b760dbe67abc3836a76eb87ffffd9de399189e/src/assets/scripts/client/aircraft/StripView/StripViewModel.js#L166

So:

Consequently, this fails to free up the CID from use:
https://github.com/openscope/openscope/blob/39b760dbe67abc3836a76eb87ffffd9de399189e/src/assets/scripts/client/aircraft/StripView/StripViewController.js#L264

Quick and dirty patch I'm applying via console as a temporary measure:

aircraftController._stripViewController._removeCidFromUse = function (cid) {
    this._cidNumbersInUse = this._cidNumbersInUse.filter(val => val !== Number(cid));
}