Improve processor stopped state management

[jalaziz]

• Introduce a new Stopped state for processors. Since a processor cannot be restarted after it has been run, this state better indicates that the processor has reached its final state.
• Expose a new Done() method on the processor to allow waiting on a processor to complete. This avoids the need to apply additional "done" channels on top of the processor when processors in go routines.

[frairon]

Thanks for your contribution @jalaziz, sorry for the delay!

[frairon]

😄 the deadlock is removed, but the test reports a data-race.

[jalaziz]

@frairon So actually, there is a data race.

The data race happens because the channel will be closed before the error is returned from Run.

I could fix the test by introducing an extra done channel for the text. However, another way to fix this is to have the done channel return the error that Run returns. This is kind of nice as you can handle the error asynchronously.

What do you think?

[frairon]

True that, there is a data race, but the data race is in the test only. If we add a synchronizing channel or a mutex it's fixed. But of course, that's not optimal.

On the other hand, returning the error through the done-channel also doesn't work, because only the first consumer might get the error. As soon as multiple clients wait for the processor it becomes inconsistent. We could create a new errors-channel each time the method is called and copy the error, which is even uglier.

So what if we instead provide an Error() method on the processor that returns the last error? Clients could call when either Run() returned or the channel from Done() is closed, if they call it earlier - they'll get a race-error in their tests. Or we protect it with a mutex to be sure.

What do you think about that?

[jalaziz]

@frairon I have implement what you suggested. Let me know what you think.

I'm a bit torn on whether all the tests should be updated to use Done() and Error(). Happy to update the tests if you think they should.

[frairon]

@jalaziz thanks for this, really! Since we have the Done()-method now, I think it does make sense to use it in the tests. I mean why do the dance with the defer close(done) everywhere if you don't have to. But I'd understand if you don't want to change all the tests. But if you do - be my guest :).

[jalaziz]

@frairon Updated the tests, but I added two tests to make sure the tests still covered the returned error.

[frairon]

Looks good, thanks! @norbertklawikowski what do you think?

[norbertklawikowski]

LGTM, just a tiny comment :)

[norbertklawikowski]

Nice, thanks a lot!