Removing a cache item requires a write lock.

[lcapaldo]

Requirements

• I have added test coverage for new or changed functionality
• I have followed the repository's pull request submission guidelines
• I have validated my changes against all supported platform versions

Related issues

Provide links to any issues in this repository or elsewhere relating to this pull request.

Describe the solution you've provided

I have deferred the removal of the "INIT_CHECKED_KEY" until we hold the write lock. If multiple threads call LDStoreInitialized concurrently it will no longer be possible to double free or otherwise corrupt the hash structure.

Describe alternatives you've considered

Not using the key at all any longer could be a simple possibility. This could introduce performance regressions if folks have stores with slow initialized implementations though.

Additional context

Add any other context about the pull request here.

[lcapaldo]

The build failure on Windows is apparently due to ftp.pcre.org being retired:

Note that the former ftp.pcre.org FTP site is no longer available. You will need to update any scripts that download PCRE source code to download via HTTPS, Git, or Subversion from the new home on GitHub instead.

[cwaldren-ld]

Hi @lcapaldo, really appreciate the fix. I'm reviewing this with the team.

Filed internally as 134551.

[cwaldren-ld]

Hi @lcapaldo! Small update for you - we'd like to add data-race detection to our test suite so that we can verify there are no regressions when this particular bug is fixed. I'll hold off integrating the fix until that is in place.

Would you mind providing a little context on the multi-threading use case that exposed this bug?

[lcapaldo]

Would you mind providing a little context on the multi-threading use case that exposed this bug?

The use case is basically some work is being parallelized across multiple threads, so they are are doing roughly the same thing on different ranges of data, and there's some behavior that's controlled by a flag that is checked in each of threads. Checking the flag could be lifted to be before the threads are created, but that's not a general solution, and doesn't allow for the possibility of updates while the process is running.

We're using a custom store (or else this code path doesn't get hit) in LDD mode because the architecture is such that we can have relatively short lived processes that are created relatively frequently (not quite PHP style "a new process per request", but somewhat similar), so we want to be able to grab the flag values closer to the processes.

If LDClientInit times out, when the various variation functions call into LDStoreInitialized and it fails, it uses the cache to keep from constantly calling initialized on the store backend. Not caching this would be acceptable for initialized for our store, I don't know about other's implementations.

To be fair, we have somewhat aggressive setting here, for both LDClientInit and the cache ttl, since we're taking an advantage of an out of process cache, which is generally quite quick to read. That said, fiddling with the timing can only change the frequency at which this occurs, not ensure it that it doesn't. Using fallthrough variations in this case is perhaps not ideal, but workable, crashing due to a double free not so much.

[cwaldren-ld]

Hi @lcapaldo, thank you for bringing this bug to our attention and for your detailed explanation.

This issue should be resolved in the latest release, v2.5.0. Do let us know if you still encounter issues.