Handle cyclic dependency between RCEA and Flags

[jrhee17]

Motivation:

A cyclic dependency can be introduced while RequestContextStorageProvider is being initialized.
A possible scenario is as follows:

Given that a user defined RCEA and enabled Flags logging:

1. A user tries to call Flags#requestContextStorageProvider
2. Before Flags#requestContextStorageProvider is fully initialized, a log is appended

   armeria/core/src/main/java/com/linecorp/armeria/common/Flags.java

   Line 1392 in 6585634

   logger.info("{}: {} ({})", flagName, value, provider.name());

3. RequestContextExportingAppender#append is invoked, which eventually calls

   armeria/core/src/main/java/com/linecorp/armeria/common/logging/RequestContextExporter.java

   Line 204 in 6585634

   final RequestContext ctx = RequestContext.currentOrNull();

4. The above call tries to initialize a new instance of RequestContextUtil, which again calls Flags#requestContextStorageProvider. null is returned from this call which results in an error.

   armeria/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java

   Lines 65 to 70 in 6585634

   	final RequestContextStorageProvider provider = Flags.requestContextStorageProvider();
   	try {
   	requestContextStorage = provider.newStorage();
   	} catch (Throwable t) {
   	throw new IllegalStateException("Failed to create context storage. provider: " + provider, t);
   	}

Modifications:

• Define a FlagsLoaded class, which contains a static variable loaded representing whether Flags has been completely loaded.
• Add a static initializer at the end of Flags which sets FlagsLoaded#loaded to true via FlagsLoaded#set.
• Check FlagsLoaded#get from RequestContextExportingAppender and don't proceed with the appender if not ready
• Add an isolated integration test

Result:

• Cyclic dependency between Flags and Logging (RequestContextExportingAppender) #4287
• An exception due to cyclic dependency isn't thrown for users who use RCEA and log Flags.

[codecov]

Codecov Report

Merging #4285 (a3ed623) into master (3c61331) will increase coverage by 0.02%.
The diff coverage is 62.50%.

@@             Coverage Diff              @@
##             master    #4285      +/-   ##
============================================
+ Coverage     73.34%   73.36%   +0.02%     
- Complexity    16967    16992      +25     
============================================
  Files          1450     1451       +1     
  Lines         64055    64126      +71     
  Branches       8046     8068      +22     
============================================
+ Hits          46983    47048      +65     
- Misses        12978    12981       +3     
- Partials       4094     4097       +3     

Impacted Files	Coverage Δ
...ommon/logback/RequestContextExportingAppender.java	66.66% <0.00%> (-1.97%)	⬇️
...c/main/java/com/linecorp/armeria/common/Flags.java	66.56% <100.00%> (+0.20%)	⬆️
.../linecorp/armeria/internal/common/FlagsLoaded.java	100.00% <100.00%> (ø)
...ecorp/armeria/dropwizard/ArmeriaServerFactory.java	70.27% <0.00%> (-8.07%)	⬇️
...rmeria/internal/common/kotlin/ArmeriaKotlinUtil.kt	35.29% <0.00%> (-6.38%)	⬇️
...p/armeria/common/stream/FuseableStreamMessage.java	81.36% <0.00%> (-1.87%)	⬇️
...rp/armeria/common/stream/DefaultStreamMessage.java	89.00% <0.00%> (-1.58%)	⬇️
...eria/internal/common/AbstractKeepAliveHandler.java	78.68% <0.00%> (-1.10%)	⬇️
...rmeria/internal/client/grpc/ArmeriaClientCall.java	80.73% <0.00%> (-0.41%)	⬇️
...armeria/internal/common/CancellationScheduler.java	78.17% <0.00%> (-0.40%)	⬇️
... and 23 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c61331...a3ed623. Read the comment docs.

[ikhoon]

How about prefixing Flags to tell what could have cyclic dependencies.

Suggested change

	public class CyclicDependencyTest {
	class FlagsCyclicDependencyTest {

[jrhee17]

Forgot about this comment 😅 Will handle renaming soon

module: rcea -> flags-cyclic-dep (since I doubt other tests will be added in this module)
test class: CyclicDependencyTest -> FlagsCyclicDependencyTest
testn ame: basicCase

[ikhoon]

Should we defer the creation of exporter until isInitialized() is true?

[ikhoon]

; so that isInitialized() is only invoked when exporter is null.

[jrhee17]

I'm unsure if I understood your intention 😅
Doesn't the current change defer setting a value to exporter until isInitialized() == true by returning early already? I saw your second comment late, makes sense 👍
e.g.

        if (exporter == null) {
            if (!isInitialized()) {
                return
            }
            exporter = builder.build();
        }

[minwoox]

Thanks! 👍

[ikhoon]

Thanks, @jrhee17!

[ikhoon]

Looks good with the new changes!

[trustin]

Thanks for fixing this, @jrhee17!

[trustin]

Please update the PR description before merging.

[jrhee17]

Please update the PR description before merging.

Oops 😅 Updated the Modifications section to reflect recent code changes 👍