use copy-on-write list in InMemoryAppender

[zhenxiao]

Fixes #8759
@leventov

[leventov]

1. It's not copy-on-write (as the title of your PR says), it's synchronizedList, but this doesn't solve the problem: see https://github.com/code-review-checklists/java-concurrency#synchronized-collection-iter. However, if you actually use CopyOnWriteArrayList instead of ArrayList as logEvents, that would help. Don't forget to change the type of the field from List<LogEvent> to CopyOnWriteArrayList<LogEvent> and to add a comment explaining the need for thread safety (in the form of using CopyOnWriteArrayList), along the lines of https://github.com/code-review-checklists/java-concurrency#justify-document.

[zhenxiao]

thank you, @leventov
comments addressed

[leventov]

This comment doesn't achieve its goal: to help somebody to see why the enclosing class needs to be thread-safe. With the current comment, in order to do that, the reader (developer) should invoke "Find usages" IDE action and analyze these usages carefully. This is time-consuming. In essence, there should be concurrent access documentation, as described in https://github.com/code-review-checklists/java-concurrency#justify-document.

This comment is also misleading because there are no critical sections in this code.

[zhenxiao]

how about:
logEvents has concurrent iteration and modification in CuratorModuleTest::exitsJvmWhenMaxRetriesExceeded(), needs to be thread safe

[LoriLori]

I am learning from better developers and was thinking about contract of logger and how would it could be solved should it be inmemorylogger be used somewhere else some day.
Isn't copy on write an overhead and a bit teaching inappropriate use of copy on write?
Isn't logEvents.toArray() atomic appropriate?
return Collections.unmodifiableList(Arrays.asList((LogEvent[]) logEvents.toArray()));
Code readability is lower and would require a comment maybe.

[leventov]

@LoriLori logEvents.toArray() doesn't solve the race condition - it may still race with an addition to logEvents and throw some exception, unless you make both append() and getLogEvents() synchronized.

Since this class resides in tests, not production, the performance doesn't matter and we shouldn't waste time comparing synchronized and CopyOnWriteArrayList. While CopyOnWriteArrayList solution is a bit simpler.

[leventov]

@zhenxiao please merge the master branch into your PR branch to make the CI pass.

[zhenxiao]

master merged into this PR. failure seems not related

[leventov]

@zhenxiao don't rebase your PRs. Your should never do this in Druid. Once you open a PR, you only merge other branches into your PR branch, never rebase it.

[zhenxiao]

oh, sure. seems tests passed