Re: async-calls and Dispatchers

On Thu, 2007-12-20 at 11:00 -0700, Alex Rousskov wrote:
> On Fri, 2007-12-21 at 04:25 +1100, Robert Collins wrote:
>
> > The dispatcher design has a great deal to do with handing results from
> > async operations, because the OS can and will call directly back into
> > user code.
>
> I am sorry, but a class with a single pure virtual method cannot have a
> great deal to do with anything, especially when there is another class
> that provides the same interface.

I presume then that the bit of my email you didn't reply to, which
suggested making one of the two interfaces a subclass of the other did
fit your concerns?

> It is possible that you planned it to be something else, but those plans
> did not materialize. In reality, we had two nearly identical classes
> that were used nearly identically. The only difference was the timeout
> setting. In practice, all those dispatchers were just a complex way to
> say "call me at least once per main loop" and we already have an
> interface for that.

By which you mean the AsyncEngine interface?

> Yes, the OS will call user code, but comm will receive such calls and
> propagate them using standard async calls. There is no need to expose
> the rest of Squid to these low-level details.

The rest of squid wasn't exposed - the most it should ever get is a
dispatcher instance to hand in when making a async call. And without
some sort of object to point to we're back to being unable to build
little tools out of the squid code base because of globals cross
referencing just about everything under the sun.

> > Re-reading the native async calls wiki page, it seems to me that 'all'
> > you need to do is implement your 'TheCalls' as a dispatcher into the
> > current EventLoop. If you are not handling the os level concurrency
> > interface, then a Dispatcher should be all you need.
>
> Yes, I could have done that. However, since async calls remove the need
> for all existing classes that were dispatching some events (or
> pretending to do so), there is no need to build upon that unused
> interface. I simply do not see any practical reason for that interface
> to exist. If you can give me specific examples where the code would
> benefit from the abstract CompletionDispatcher class (and should not use
> the remaining AsyncEngine API), I am all ears.

Well. If everything is becoming an AsyncEngine then *an* interface which
is modular is being preserved; we can always split it back out again
when someone wants to have the flexibility it offers; Please do read the
'proactor' ACE pattern if you haven't.
http://www.cs.wustl.edu/~schmidt/PDF/proactor.pdf

As for concrete reasons to have dispatcher and engine separate:
 - code reuse (all engines of a similar type/behaviour can be given the
same dispatcher)
 - scaling (as we start working towards multiple core in the future a
single engine will correctly dispatch back to the originating thread or
process because the dispatcher instance given is for that thread)
 - loose coupling - we can experiment with different async call
mechanisms without having to do an all-at-once conversion, by just
adding a different dispatcher instance for the code that needs that.

But looking at the patch I can see that all you've done is move the
abstract interface one level further out. So the entire discussion is
really moot - I don't care if its a level further out or not, though I
think the extra indirection you are adding via
AsyncCallQueue::Instance().fire() is more complexity not less.

I presume you've made the signal handler dispatcher into an engine, or
will do so.

More importantly I can't see any unit tests. If we want to end up with a
rigourously tested code base - which is both well worth doing and
something I thought we as a group were interested in - replacing tested
code with untested code is a step backwards.

> > Note also that the
> > current design was in the process of removing global state to make
> > testing and modularisation easier, so please do take care not to
> > reintroduce dependencies on global variables etc.
>
> From that point of view, we are essentially removing things here, not
> adding new ones so modularization can only improve. As for unit testing,
> if it makes the actual code a lot more complex, it is doing more harm
> than good.

I'm not sure if you are saying that you are adding global variables
because its 'simpler' (its not, and I'm *really* against more
globals)... or just raising a hypothetical point about tradeoffs: to
which my matching response would be:

Trying to convert untestable code into testable code will tend to push
existing complexities and mispractices into much greater visibility,
while at the same time the regions that are well tested become
substantially easier to reuse (because tests reuse them).

If converted code is more complex to use then indeed there is something
wrong and we should look more closely but that may speak more to how the
code was refactored to allow testing than to the fact its being tested.

-Rob

-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.