Fix memory leak in thread-local VM & plugin caches

[phmx]

Since VM and plugin thread-local cache keys include volatile parts (namely VM configuration, code and plugin configuration), their reconfiguration/update (e.g. with Envoy ADS protocol) might lead to memory leak by leaving those thread-local map stale keys behind. The current cleanup method is insufficient, as it accounts only for cache hit case.

Also, plugin configuration is placed into cache key as is (by contrast with SHA256 hash for VM keys), which causes noticeable memory consumption when configuration is large.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[PiotrSikora]

@mpwarres @martijneken @kyessenov could you review this change and verify that it fixes the issue? Thanks!

[kyessenov]

Yeah, the issue is real, I was aware of it. Generally LG.

[PiotrSikora]

Could you split this PR in two? The SHA256 and cache cleanup are completely separate features, so it will speedup review and merging.

[phmx]

Could you split this PR in two? The SHA256 and cache cleanup are completely separate features, so it will speedup review and merging.

Sure, here you go -- #359 . This one's going to be all about key leak/cleanup.

[mpwarres]

LGTM mod minor comments. Thanks!

[mpwarres]

LGTM, thanks! @PiotrSikora and @kyessenov, does the current PR address your comments?

[PiotrSikora]

Any chance for a test, or is it hard to exercise?

[phmx]

Any chance for a test, or is it hard to exercise?

There are a couple of calls to getOrCreateThreadLocalPlugin in test/wasm_test.cc triggering new code, but it's not enough of course.
If it's acceptable, I could declare (in src/wasm.h) several testing-only functions to have access to an internal cache state (e.g. to check whether some key is still present). Btw. one such function clearWasmCachesForTesting is not called anywhere, might be a candidate to remove 🤔 UPD: Ah, it's called by the Envoy tests, makes sense.

[mpwarres]

If it's acceptable, I could declare (in src/wasm.h) several testing-only functions to have access to an internal cache state (e.g. to check whether some key is still present).

Adding testing-only functions to access cache state SGTM, thanks!

[PiotrSikora]

LGTM, sans nits.

But I'd also like to hear from @kyessenov.

[phmx]

If it's acceptable, I could declare (in src/wasm.h) several testing-only functions to have access to an internal cache state (e.g. to check whether some key is still present).

Adding testing-only functions to access cache state SGTM, thanks!

Sorry for delay. I'll think of something. Maybe I'll just move removeStaleLocalCacheEntries into a header and test it in isolation 🤔

[PiotrSikora]

I'll think of something. Maybe I'll just move removeStaleLocalCacheEntries into a header and test it in isolation 🤔

FWIW, my original ask was for a test that's broken (showing a leak) without this PR and fixed with this PR. Personally, I don't see too much value in unit testing removeStaleLocalCacheEntries, but I'll leave this to @mpwarres.

[kyessenov]

LG, thanks for bounding it.

[mpwarres]

I'm good with this as-is. Thanks for the contribution!

[phmx]

I'm good with this as-is. Thanks for the contribution!

@mpwarres and team, thank you so much for review! Sorry for being late with tests. Could you please take a look at them in #362 ?