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#35nickdnk wants to merge 3 commits intoclugg:masterfrom nickdnk:fix_leaks
DeepCopy problems for objects with ordered keys to prevent snapshot leaks on copy#35nickdnk wants to merge 3 commits intoclugg:masterfrom
nickdnk:fix_leaks
Conversation
Merge problems for objects with ordered keys + prevent snapshot leaks on copyMerge problems for objects with ordered keys to prevent snapshot leaks on copy
Merge problems for objects with ordered keys to prevent snapshot leaks on copyDeepCopy problems for objects with ordered keys to prevent snapshot leaks on copy
nickdnk
commented
Mar 23, 2023
nickdnk
commented
Mar 23, 2023
nickdnk
commented
Mar 23, 2023
| if (type != JSON_Type_Object) { | ||
| continue; | ||
| if (isArray) { | ||
| view_as<JSON_Array>(result).Concat(view_as<JSON_Array>(obj)); |
Contributor
Author
There was a problem hiding this comment.
We can still use Concat here since it's only Merge that's not working.
nickdnk
commented
Mar 23, 2023
| public int Iterate() | ||
| { | ||
| if (! this.OrderedKeys) { | ||
| if (! this.OrderedKeys && ! this.IsArray) { |
Contributor
Author
There was a problem hiding this comment.
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
Contributor
Author
|
Rebased on #33 just to keep myself sane. Still draft. |
Owner
|
Thank you! Please see #34 (comment) |
Owner
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
TrieSnapshots when DeepCopying via forwards in other plugins.Mergegets 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 } }{ "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
Mergedoesn'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.