fix default event port behavior along with FileDescriptorActivity

[doudou]

With the introduction of the work() method, which allowed to
be more precise on what gets called in which condition,
an event port under FileDescriptorActivity would not make
updateHook get called anymore.

This broke the Rock workflow with file descriptor activities.
(Un)fortunately, the effect has been subtle enough to not be
noticed readily. Or people started deploying tasks under the
normal Activity where before it would work with the FDA (I'm
guilty of that).

This is a rather convoluted code path (the characters in the
play are a FileDescriptorActivity 'FDA', ExecutionEngine 'EE'
and a TaskContext 'TC'

• the port signal calls FDA::trigger()
• FDA::trigger() wakes-up the FDA loop, which calls EE::work(Trigger)
• EE::work(Trigger) calls EE::processPortCallbacks but NOT
  EE::processHooks (expected from the work() refactoring)

At that point, we have to remember that TC::addEventPort registers
a port callback that by default calls TC->trigger(). The
comment at that point in the code itself says "default schedules
an updateHook" (which we're indeed expecting with a FDA)

• so, EE::processPortCallbacks in the end calls TC::trigger
• which, by default calls FDA::timeout()

Until now, FDA::timeout() was not implemented. This PR implements it.

[snrkiwi]

The first commit has a mix of whitespace fixes, refactoring, and some actual changes. Can you indicate what the actual changes in behavior are? It's hard to pull that out of the noise ...

[doudou]

Until now, FDA::timeout() was not implemented.

This is the change: FDA::timeout is now implemented so that work(Timeout) gets called.

[meyerj]

Unfortunately I don't have time at the moment to check in detail, but please check my existing PR which partially refactored the FileDescriptorActivity implementation in #309. The primary goal was to get rid of the locks in trigger() and breakLoop() and it should not affect the behavior of trigger() with respect to which hooks get triggered.

With your patch FileDescriptorActivity basically becomes a normal non-periodic Activity that can additionally be triggered by file descriptor activities or timeouts. That is not exactly the behavior that I would expect, and I would have assumed that it was not the intention in the original implementation. But of course it is possible, and if you as one of the original authors think that it should be like that, it's fine.

It is correct that FileDescriptorActivity::trigger() and EE::work(Trigger) do not and should not trigger a full update cycle including updateHook(). The activity gets triggered way too often, for example whenever a called operation executed in another TaskContext finished and wants to notify the caller. Also note that TaskCore::trigger() actually calls getActivity()->timeout() in TaskCore.cpp:93 since #91, not getActivity()->trigger(), to not change the semantics of this method or break the API, but the difference is very subtle and not very obvious. Maybe we should have already introduced new, more explicit methods to TaskCore and deprecate TaskCore::trigger() at some point later...

[doudou]

@meyerj thanks for the long explanation. It nicely confirms the data flow I thought was happening, always good.

The original behavior was intentional. This is a major breakage for rock. I did understand the intent of the change to work, and am definitely not contesting that. This is meant to restore pre-work behavior that we rely on.

As for the other pull request ... I'm not sure what you expect me to do with it (except from reviewing it, which I just did)

[meyerj]

@meyerj thanks for the long explanation. It nicely confirms the data flow I thought was happening, always good.

The original behavior was intentional. This is a major breakage for rock. I did understand the intent of the change to work, and am definitely not contesting that. This is meant to restore pre-work behavior that we rely on.

Okay. I do not see an immediate problem is we still support user triggers with TaskCore::trigger() or FileDescriptorActivity::timeout(), and the unit test makes sense and passes, too.

As for the other pull request ... I'm not sure what you expect me to do with it (except from reviewing it, which I just did)

Yes, exactly. I was not sure anymore whether it would conflict conceptually or maybe already solved your problem in another way. But apparently this is not the case and the small merge conflicts can be resolved easily.

So LGTM!