[squid-dev] Convert store_client into AsyncJob

[Alex Rousskov]

On 2024-12-27 01:07, Shailesh Vashishth wrote:

> I am trying to do the following ToDo in store_client.cc.
> // TODO: Convert store_client into AsyncJob; make this call asynchronous

I do not recommend working on that to-do:

1. It is too complex, requiring good understanding of AsyncJob design 
that may be difficult to obtain through reading documentation alone.

2. It may be premature because we probably should "Merge store_client 
into StoreClient" first (or at the same time), as documented in another 
TODO in StoreClient.h.

3. The change requires serious testing that would be difficult to 
perform and nearly impossible to validate via review.


> storeClientCopy2(this, sc);

> Please help me understand why we need to make this call asynchronous?

The relevant StoreEntry::invokeHandlers() code is best interpreted as 
the following pseudo-code loop:

     for (const auto &client: entry->store_clients)
         client->do_something_complex_with(entry);

In other words, the code is contacting individual store_client tasks in 
a loop while asking each task to perform complex actions with numerous 
side effects, including cases where an action of one task affects 
another task (and/or the looping/entry object itself!).

This is essentially spaghetti code in disguise, where (very indirect!) 
recursive function calls and same-object recursive "visits" have 
replaced classic "goto" statements: 
https://en.wikipedia.org/wiki/Spaghetti_code

Experience shows that humans are terrible at writing and especially 
maintaining correct spaghetti code. This specific invokeHandlers() loop 
has caused many costly problems! Not all of them have been solved IIRC.


Asynchronous calls untangle this mess because they prevent recursive 
calls and revisits. They have their own cons, of course, but they are a 
part of a much better overall design.


HTH,

Alex.