Fix NPE in TestRunSession#toString and use AtomicReference

[laeubi]

Currently if one calls TestRunSession#toString (e.g. in a debugger) a
NPE can occur if the session is not yet started and the fStartTime is
null. Also it is possible that fStartTime is concurrently set.

This checks for this case adjust the message accordingly and also
replace the volatile field with an AtomicReference to ensure the start
time is only ever set by the first thread, alongside with that an
unnecessary synchronized is removed from the method
addTestSessionListener as ListenerList is already synchronized.

[github-actions]

Test Results

1 154 files   -   577  1 154 suites   - 577   51m 54s ⏱️ - 31m 22s
4 004 tests ±    0  3 981 ✅  -     1  23 💤 + 1  0 ❌ ±0 
8 406 runs   - 4 203  8 311 ✅  - 4 137  95 💤  - 66  0 ❌ ±0 

Results for commit b04304b. ± Comparison against base commit a2ad0b3.

This pull request skips 1 test.

AutomatedResourceTests AllRegressionTests Bug_329836 ‑ testBug

♻️ This comment has been updated with latest results.

[jukzi]

There is no need for AtomicReference
"This class is immutable and thread-safe."
https://docs.oracle.com/javase/8/docs/api/java/time/Instant.html

the rest looks good.

[laeubi]

There is no need for AtomicReference "This class is immutable and thread-safe." https://docs.oracle.com/javase/8/docs/api/java/time/Instant.html

the rest looks good.

The class is threadsafe, but not the access to its instance.

[jukzi]

The class is threadsafe, but not the access to its instance.

That was done by the volatile keyword.

[laeubi]

The class is threadsafe, but not the access to its instance.

That was done by the volatile keyword.

Even though I explained it already multiple times, volatile keyword does not help you in any way with concurrency problems. volatile only tell the jvm that it should not assume that reading a variable once does not change it in a local context, eg. with

while(!this.stop)  {
  //do something
}

then this.stop must be volatile if it can be altered by another thread because otherwise the jvm is allowed to read the value stop once and if the loop itself does not alter the value you can end up in an endless loop. But this does not prevents two threads to alter the value at all.

If one needs this (and that's the case here) you need an atomic compare-and-set operation, what is offered by the AtomicXXXX set of classes and is used here in testRunStarted

[jukzi]

There is no need to prevent two threads from writing into it in this case. Even if both multiple threads write a value it would be the same timestamp +-1. Here is no condition that relies on a atomic write.

[laeubi]

There is no need to prevent two threads from writing into it in this case. Even if both multiple threads write a value it would be the same timestamp +-1. Here is no condition that relies on a atomic write.

It is this code badly fails in some cases when test are running to fast or from different threads, so why should I want a weaker guarantee if I can have a stronger one that can be used with more features. volatile is about memory effects, AtomicReference is about concurrency and I want to solve concurrency problems here.

[jukzi]

It is this code badly fails in some cases when test are running to fast or from different threads,

That is not reflected by the commit message/PR title. If you list a problem that is solved by this change it would be OK for me. But it seems to be totally over engineered to fix the NPE during debug.

[laeubi]

I adjusted the commit message now.

[fedejeanne]

Just one comment on a coding practice, the rest looks good.

Thank you, @laeubi , for contributing this!

[fedejeanne]

Move outside else