Fix config being loaded twice #293
Conversation
| */ | ||
| private static final class CountingAgentConfigSupplier implements Supplier<Config.AgentConfig> { | ||
|
|
||
| private final AtomicInteger count = new AtomicInteger(0); |
There was a problem hiding this comment.
This AtomicInteger is probably overkill, but I left it in on the off chance someone wants to expand on this test case later. I originally wrote this test to try to reproduce the double-checked locking race condition that can occur by using a multi-threaded scheduled executor service to ask for the AgentConfig 10,000 times via separate callables all calling get on the same AgentConfigSupplier instance and verifying that the inner CountingAgentConfigSupplier was only invoked once. I wasn't able to reproduce the bug in that fashion, so I pulled out the test case and simply tested the supplier "the normal way". Since CountingAgentConfigSupplier is initialized BeforeEach, there isn't necessarily a need to have this class be thread-safe and we could simply make this an int. Happy to back it out to avoid complexity or keep it in if we want to expand the test cases around this more.
| @@ -51,7 +51,8 @@ private HypertraceConfig() {} | |||
| // so avoiding for perf reasons | |||
| private static volatile boolean servletCausingException; | |||
|
|
|||
| private static AgentConfig agentConfig; | |||
There was a problem hiding this comment.
Thanks for putting this together. I like the testability, but on the other hand, I would like to keep it as simple as possible. Would it be enough to just use volatile on this property and not use the supplier approach?
There was a problem hiding this comment.
Yeah, no problem! I think in addition to making this field volatile, we would also want to synchronize the write to this field in HypertraceConfig.reset() unless you're not worried about this class being thread-safe when APIs exposed for testing purpose are in use.
pavolloffay
changed the title
…onfigSupplier" This reverts commit 19ad84b.
|
thanks @ryandens |
Description
Resolves #223 (kind of). This bug no longer seems to be present, but as a result of the investigation into #223 a small thread-safety issue was noticed in the double-checked locking implementation. To fix it, I pulled the implementation of memoizing the
AgentConfiginto a separate class. This made it easier to inject a test double (specifically, a spy) to verify that the config is properly memoized.Testing
Added test cases on the memoization and resetting of the memoized value.
Checklist:
Documentation
No new documentation required
This solution was based off of/inspired by