Uh oh!
There was an error while loading. Please reload this page.
Make AztecAttributes immutable in order to stop crashes - #751
Conversation
…/removing attributes
malinajirka
left a comment
There was a problem hiding this comment.
Thanks @planarvoid ! 👍 for making the AztecAttributes immutable!
LGTM! I've added couple of minor comments. We might also consider overriding add/remove Atrribute and making them deprecated. Wdyt?
| return aztecAttributes | ||
| } | ||
| fun withValues(keyValues: Map<String, String>): AztecAttributes { |
There was a problem hiding this comment.
Have you considered calling withValue(keyValue: Pair<String, String>) in a loop? Creating a short living object (Pair) in a loop shouldn't have any performance impact.
There was a problem hiding this comment.
we'd also be creating a copy of AztecAttributes every time we call withValue. I'd rather do it the other way around and create a short living map and call withValues from within withValue, what do you think?
| fun withoutValues(keys: List<String>): AztecAttributes { | ||
| val aztecAttributes = AztecAttributes(this) | ||
| for (key in keys) { |
There was a problem hiding this comment.
Have you considered calling withoutValue(key: String) in a loop?
planarvoid
commented
Oct 15, 2018
@malinajirka No idea why the tests were failing. It's nothing I've touched. I've replaced two methods (withValue/withValues) with one with vararg params. Let me know what you think. |
malinajirka
commented
Oct 16, 2018
@planarvoid These |
malinajirka
left a comment
There was a problem hiding this comment.
LGTM - feel free to merge it when Travis finishes
…es in case they get modified during listener calls, needed after attributes immutability change in PR #751
This crash (in theory) shouldn't happen. This leads me to thinking that two independent threads are modifying the same
AztecAttributesobject and there is a race condition when setting/removing attributes. MakingAztecAttributesimmutable and passing always a copy should fix the problem. Let me know what you think.Fix#705
Review
@0nko@malinajirka