RFR: 8267952: async logging supports to dynamically change tags and decorators

[Xin Liu]

On Thu, 10 Jun 2021 04:57:29 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> The order does matter. There is a very tricky race condition.  Here is the original order.  
>> 
>> 
>> A: LogDecorations decorations(level, *this, _decorators);
>>> B: LogOutputList::Iterator it = _output_list.iterator(level);
>> C: for (; it != _output_list.end(); it++) {
>> 
>> 
>> The context switch may happen right before stmt B.  Stmt A has been executed and it used the old _decorators. 
>> However, the global counter hasn't increased yet so `wait_until_no_readers()` doesn't take any effect because the reader counter is zero.  Another thread which is executing `LogConfiguration::configure_output` can change LogOutput::decorators.  If the user selects more decorators than old,  `LogFileStreamOutput::write_decorations`
>> will print more decorators which are not initialized in `LogDecorations` ctor.
>> 
>> The newly added LogConfigurationTest.reconfigure_decorators_MT unveils this issue. It could happen with both synchronous logging or async logging.  Tier-1 test on github workflow uses 2 cores. it's easy to trigger this problem if Linux preemptive is enabled.
>
> I understand there is a race. I'm asking you to add a comment explaining there is a race so that someone is not tempted to reorder these statements again. That said it seems to me that we also require a `loadload` barrier between these (which would pair with the storestore barrier in `LogConfiguration::configure_output`).

I see. I would like to convince reviewers first. I will add some comments here. 

I don't think loadload barrier is necessary.  volatile in c/c++ is a little bit different from Java. we can't fully apply JSR-133 here. Yes, we do need to make sure Atomic::add() happens before load of decorators. This is done by the implied memory barrier of `Atomic::add`.

-------------

PR: https://git.openjdk.java.net/jdk/pull/4408