Skip to content

Fix DeepCopy problems for objects with ordered keys to prevent snapshot leaks on copy#35

Closed
nickdnk wants to merge 3 commits intoclugg:masterfrom
nickdnk:fix_leaks
Closed

Fix DeepCopy problems for objects with ordered keys to prevent snapshot leaks on copy#35
nickdnk wants to merge 3 commits intoclugg:masterfrom
nickdnk:fix_leaks

Conversation

@nickdnk
Copy link
Contributor

@nickdnk nickdnk commented Mar 23, 2023

Hello

This is a proof-of-concept of changes that need to be applied (or, a suggestion) in order to fix #34.

Basically, there were two problems here:

  1. If you don't use ordered keys, you will leak TrieSnapshots when DeepCopying via forwards in other plugins.
  2. If you use ordered keys, Merge gets the objects wrong when deep copying, and you get invalid handle exceptions on cleanup.

I don't have a solution to preventing leaks if you don't use ordered keys, but at least in my case, I can control that on my end by enabling it on all objects I send via forwards, and receiving plugins can copy these objects without leaking, and those will then also have ordered keys.

I don't fully understand why the merge logic doesn't work with ordered keys. I experienced it setting deep-copied properties (objects/arrays) multiple times on the same object, so you'd get objects like these (if you encode them out), which makes no sense:

{
    "player": {
        "user_id": 3,
        "steamid": "76561197996426755",
        "side": "ct",
        "name": "Nyxi",
        "is_bot": false
    },
    "friendly_fire": true,
    "player": {
        "user_id": 3,
        "steamid": "76561197996426755",
        "side": "ct",
        "name": "Nyxi",
        "is_bot": false
    }
}
  • which should have been:
{
   "player": {
       "user_id": 3,
       "steamid": "76561197996426755",
       "side_int": 3,
       "side": "ct",
       "name": "Nyxi",
       "is_bot": false
   },
   "friendly_fire": true,
   "blind_duration": 4.742862
}

This PR is just a draft to illustrate how the problem can be solved. I essentially just duplicate the logic from shallow copy in the deep copy code, but deep copying objects directly then, which prevents the use of the Merge function. It's also technically faster since you don't need to loop the objects twice.

The "real" solution is probably to figure out why Merge doesn't work with ordered keys, but I just couldn't. It makes no sense to me. I should add that shallow-copying does work with ordered keys, but since you re-iterate the same object again in a deep copy, it goes wrong the second time you attempt to set keys - when overriding the shallow copied objects with new objects.

@nickdnk nickdnk changed the title Fix Merge problems for objects with ordered keys + prevent snapshot leaks on copy Fix Merge problems for objects with ordered keys to prevent snapshot leaks on copy Mar 23, 2023
@nickdnk nickdnk changed the title Fix Merge problems for objects with ordered keys to prevent snapshot leaks on copy Fix DeepCopy problems for objects with ordered keys to prevent snapshot leaks on copy Mar 23, 2023
@nickdnk nickdnk marked this pull request as draft March 23, 2023 03:23
if (type != JSON_Type_Object) {
continue;
if (isArray) {
view_as<JSON_Array>(result).Concat(view_as<JSON_Array>(obj));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can still use Concat here since it's only Merge that's not working.

public int Iterate()
{
if (! this.OrderedKeys) {
if (! this.OrderedKeys && ! this.IsArray) {
Copy link
Contributor Author

@nickdnk nickdnk Mar 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not technically necessary as we don't call Iterate() on arrays at all now (I changed it to always use Length), but in a forward, someone could still call Iterate() on an array and create snapshot leaks that way.

Support deep concat arrays
Fix doc error
Fix test for null key copy
@nickdnk
Copy link
Contributor Author

nickdnk commented Mar 24, 2023

Rebased on #33 just to keep myself sane. Still draft.

@clugg
Copy link
Owner

clugg commented Mar 25, 2023

Thank you! Please see #34 (comment)

@clugg
Copy link
Owner

clugg commented Mar 27, 2023

The issue with ordered keys/iteration problems have been fixed in v4.1.3. The issue with leaking snapshots has been fixed in v5.0.0. Thanks again for your time and effort :)

@clugg clugg closed this Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] JSON objects passed to forwards then iterated on causes memory problems

2 participants