find_property method in parser.hpp - cache having duplicated entries

[issaharnoam]

row 179, parser.hpp
needto to following fix (move caching inside if statement)
otherwise it is occasionally duplicated
...

• property_info propInfo(pBufferIndex_, currentPropInfo, propertyLength);
• cache_property(pName, propInfo);

// The property was found, return it
if (name == pName) {

• property_info propInfo(pBufferIndex_, currentPropInfo, propertyLength);

• cache_property(pName, propInfo);

  return propInfo;
  }

[zacbrown]

Hi,

Could you please provide a proof of concept or a more detailed explanation of this issue? A debug dump with full symbols demonstrating the duplicated properties in the cache would be sufficient.

krabsetw is single-threaded per trace session. We don't guarantee a unique cache across trace sessions. I'm confused as to how we might end up in a scenario where we've already added something to the cache if there's only a single thread.

thanks,

Zac

[swannman]

This sounds like the same issue that @aydany has a pull request out for.

[zacbrown]

Ah that’s right. I’ll link it.

Could you ping them internally and see if they have bandwidth to address the PR?

If not, I might be able to carve some time out to add the tests since we have a second +1 for the issue.

[issaharnoam]

After I run and tested the code, I see this fix may cause other problems (lastPropertyIndex_ is stored into member variable), the fix should be more wide... Sorry for misleading ...
There is a need for wider fix...

[issaharnoam]

why std::deque<std::pair<const wchar_t *, property_info>> is used and not std::map? it could replace all the logic with find_property etc.. and make it much simplier?

[zacbrown]

@issaharnoam - I believe historically, it was an optimization based on the observation that for a small N (number of items), traversing the deque would actually be faster than using a Binary/Hash map.

As to the fix, I'm open to an alternative if you have one.

[swannman]

@zacbrown I’ve pinged @aydany twice now with no reply.

[zacbrown]

Thanks! I’ll try to find some time then :).

[zacbrown]

Fixed in #41