Allow components that will check their status asynchronously

[Grundlefleck]

It's preferable to serve a status page quickly. If the status of components are generated synchronously, during a request, it can cause the entire request to hang or be expensive. This pull request allows wrapping normal, written-as-synchronous components to be wrapped in an AsyncComponent, that will check the status periodically, and serve the current status very quickly.

Features of AsyncComponent:

• runs synchronous status check within threads from a given Executor
• can supply a different Executor per component, to prevent backups of one component causing other components to hang
• automatically reports warning if a status has not been successfully updated recently enough
• provides a hook for when a status is updated (to allow e.g. custom logging)
• almost all parameters are configurable, with sensible defaults

Example usage (shown with defaults):

    AsyncComponent asyncComponent = AsyncComponent.wrapping(myPreexistingNormalComponent)
        .withExecutor(Executors.newScheduledThreadPool(1))
        .withRepeatSchedule(30, TimeUnit.SECONDS)
        .withStalenessLimit(5, TimeUnit.MINUTES)
        .withInitialDelay(0, TimeUnit.SECONDS)
        .withUpdateHook(AsyncComponent.Consumer.NOOP)
        .build();

(equivalent to):

    AsyncComponent asyncComponent = AsyncComponent.wrapping(myPreexistingNormalComponent).build();

This should then be given to an instance of AsyncStatusPageGenerator, similar to before:

    List<AsyncComponent> myAsyncComponents = ...
    AsyncStatusPageGenerator generator = new AsyncStatusPageGenerator("my-application-id", new JarVersionComponent(this.getClass()), myAsyncComponents)
    generator.start(); // tells all AsyncComponents to begin periodically scheduling their checks

The instance of AsyncStatusPageGenerator can then be given wherever StatusPageGenerator was previously given.

[scarytom]

Hi @Grundlefleck,

I understand what you are trying to achieve with this change, and it was something that @tomwhoiscontrary and I discussed several times.

I'm not convinced, however, that this implementation is ideal. I'm concerned that each component is coupled to the execution framework, and would personally have sought to implement by composing an asynchronous component from a normal component. I think it would be desirable set up a status page generator that takes ownership of the executor, and not give knowledge of this to the components.

Do you have any thoughts on this?

[tomwhoiscontrary]

"implement by composing an asynchronous component from a normal component" - isn't that what this does?

Granted, it also has the overarching AsyncStatusPageGenerator thingy, but you could use the async components without it. Hence, i would look for a way to do without the async generator altogether; it looks like it's principally a way to coordinate starting and stopping a bunch of async components, and that feels like a concern which should be separate from aggregating their outputs.

Bit dubious about accessing two separate atomics. Racy. Would prefer a single atomic holding a composite object, or just good old fashioned synchronized blocks - as long as the only actions inside the blocks on either side of the wall are a pair of stores and a pair of loads, there's no troubling concurrency bottleneck.

Also a bit dubious about the proliferation of single-thread scheduled executors. My gut feeling is that you want to find a way to safely manage N possibly-slow components from a single pool of M threads, where in happy times M << N. Something like a CachedThreadPoolExecutor driving the components via a facade which stops any component which is still running from being called again. 65% chance this is already in java.util.concurrent, 80% chance it's in Guava.

Anyway, REJECT, one source file has no newline at end of file, unacceptable.

Suggest getting J-Fred to sponsor a hackathon next time you're down south, @Grundlefleck, we can apply finishing @scarytom - appeasing touches then.

[Grundlefleck]

Thanks for comments. This is a first cut that got us where we needed to be, and didn't change anyone else's usage, happy to tweak it until we're all happy.

I'm concerned that each component is coupled to the execution framework

Just so we're on the same page, do you mean the Java Executor Framework (ScheduledExecutorService, TimeUnit, etc)? If so, I'm not sure coupling to that is a problem, since it's in the JDK, and for example, AtomicBoolean was already referenced within Tucker.

would personally have sought to implement by composing an asynchronous
component from a normal component

That is what the current implementation is designed to do. Users should now be able to write their components as-if-single-threaded, with getReport able to make potentially long-running synchronous calls (with some caveats of course). Based on experience writing async components within Play applications, I think this will be the easiest model to work with, because it mostly removes threading concerns from component implementations.

I think it would be desirable set up a status page generator that takes
ownership of the executor, and not give knowledge of this to the components.

Some background on why it looks as it does. We wanted to achieve two things:

• components change to a WARNING status if they have gone stale (i.e. report hasn't been updated in x minutes)
• components do not cause warnings to cascade to other, unrelated components

Consider a badly written component that fails to time out when making a network call: if threads from an underlying executor get used up as they're all waiting on this call, other components will become stale, and will change to a WARNING. The warning can be a false positive, because there's no real issue with what the component is checking. My interest is that we can achieve those two things, but I don't feel particularly strongly that the implementation has to remain the way it is.

Having said that, knowledge of the executor does not get leaked to the user-created components, only the AsyncComponent within Tucker, that decorates the user's component. I think that's reasonable. Originally I thought about adding a new generator that took normal components and handled the executor, but I suspected that would devolve to a map of components to threadpools, so found it easier to manage within a wrapper of a single component.

To summarise: I think the current design what you are suggesting. If there's a mismatch, could you explain in a bit more detail?

[Grundlefleck]

Hence, i would look for a way to do without the async generator altogether; it looks like it's principally a way to coordinate starting and stopping a bunch of async components,

It is...

and that feels like a concern which should be separate from aggregating their outputs.

... and it probably should be. The only bits of it that are about aggregating outputs just delegate to the preexisting StatusPageGenerator, which corroborates the smell. There's also a mismatch in terms of the addComponent method, since the async generator takes a list of components in the constructor. I expect there's a better way, where there's only a single implementation of a generator, and convenience methods to start and stop a list of async components.

Bit dubious about accessing two separate atomics. Racy.

Agreed. Will fix. Probably with the composite object approach you suggested.

there's no troubling concurrency bottleneck.

Worth bearing in mind that the point of this construct is to protect against slow running component checks, most likely involving network calls. Writes will likely be infrequent, with many more (fast) reads in the meantime. Given that, I wasn't particularly concerned about the performance at the level of volatile writes and reads. Let me know if you think that's negligent.

Also a bit dubious about the proliferation of single-thread scheduled executors.

Could you be more specific about what your concern is? Creating too many under-utilized threads? Unfriendly API?

Something like a CachedThreadPoolExecutor driving the components via a facade which stops any component which is still running from being called again.

AsyncComponent already does this, albeit in a naive way, only rescheduling the check when the last one completes. There's some edge cases to be worked out that I've not thought too deeply about, e.g. non-timing-out checks causing a warning that can't be reset. That would still have to be addressed either way.

Given the concern I mentioned in an earlier comment, to guarantee a misbehaving component does not cause failures in other, unrelated components, I don't immediately see a way to achieve that with # threads < # components. Suggestions welcome.

Anyway, REJECT, one source file has no newline at end of file, unacceptable.

I will LITERALLY never forgive myself.

[Grundlefleck]

Just realised that this pull request (even with pending rework) could satisfy issue #3.

[Grundlefleck]

FYI just had a good chat with @jheister about some changes, which match pretty closely with @tomwhoiscontrary's points, so don't waste too much time thinking about it until you see the result of those changes, which I'm about to start work on.