Chronicle-Threads icon indicating copy to clipboard operation
Chronicle-Threads copied to clipboard

The contract for EventHandler#loopFinished is violated a lot

Open nicktindall opened this issue 3 years ago • 2 comments

The javadoc for EventHandler#loopFinished states that

Event loop implementations call this once only, from the event loop's execution thread.

But there are lots of places we call it from threads other than the event loop's execution thread. I think we should clarify the contract also

  • It says "this is an appropriate place to perform cleanup" but with the new event handler lifecycle that should really be in close
  • Does it still get called if the loop was never started? I assume no because it should only be called from the "event loop's execution thread"
  • If it doesn't get called when the loop is never started then it's definitely not "an appropriate place to perform cleanup"

nicktindall avatar Aug 11 '22 04:08 nicktindall

We should check that loopStarted is also called in event loop thread too.

The reason for the cleanup comment is for historical reasons - TcpEventHandler used to close its Bytes in close() and if that happened before action() was called then it could access released memory and crash the JVM. We now have a better solution - e.g. the recommendation to throw InvalidEventHandlerException from action if the EventHandler is closed, so maybe we should review the doco for loopFinished to remove the "this is an appropriate place to perform cleanup"

JerryShea avatar Aug 11 '22 04:08 JerryShea

We should check that loopStarted is also called in event loop thread too.

There's already tests for that, it is

nicktindall avatar Aug 11 '22 04:08 nicktindall