squid icon indicating copy to clipboard operation
squid copied to clipboard

Replaced most custom high-level callbacks with a unified API

Open rousskov opened this issue 1 year ago • 0 comments

Regular asynchronous calls remember call arguments at call creation time, when the type of the call is well known. Service callbacks do not know call arguments until it is time for the service to (asynchronously) call the previously supplied callback. Thus, the service has to get access to call parameters inside the stored generic (from service p.o.v.) async call pointer. Services got that access by declaring a custom callback dialer class and expecting the service user code to use that dialer when creating an async call object. That was problematic:

  1. Each service class had to create nearly identical dialer classes, at least one for each unique answer type. Code duplication was rampant.
  2. Each service dialer class was specific to one service user type (e.g., HappyConnOpener answer dialer did not support AsyncJob users).
  3. A compiler could not check whether the service got the right callback: A dynamic_cast operation converted a generic dialer to the service-declared dialer class, asserting the right type at runtime.

This change replaces all but one custom service callback with three general service-independent dialers, one for each callback style: a job call, a call to a cbdata-protected class method, and a stand-alone function call. This unified API solves problem 1 mentioned above. A new Callback template supports all three dialers (addressing problem 2) and checks the callback answer type at compile time (addressing problem 3).

Replaced custom HappyConnOpener, Security::PeerConnector, Downloader, CertValidationHelper, Http::Tunneler, Ipc::Inquirer, and most Ipc::StartListening() callbacks (and more such callbacks would have been added by upcoming PRs!). Kept ListeningStartedDialer as detailed below.

Replaced Ipc::Inquirer callback with a regular call: The class does not need a callback mechanism because it can (and should and now does) create an async call from scratch when the call arguments become known.

Kept ListeningStartedDialer because client_side.cc relies on delivering additional, non-answer parameters with the callback. Normally, this additional information is stored inside the job object. In this case, no job awaits the port opening, so we use a custom dialer hack to keep that information during the wait. Improving that code is out of scope. This may be the only custom high-level callback left in the code!

The asyncCall() return type improvement preserves the exact AsyncCall type for the Callback constructor to pick up. It will also remove explicit/imprecise AsyncCall::Pointer uses throughout the code.

Also fixed reporting of small responses received by Downloader.

Unfortunately, we cannot easily unify the old JobCallback() with the new asyncCallback() because we do not yet know how to detect that we need to use one of the Comm-specific dialers (e.g., CommCbMemFunT) that feed the job pointer to the CommCommonCbParams constructor. CommCommonCbParams do not have a default constructor because they require, probably incorrectly, that the recipient (e.g., job) pointer is stored together with the future callback argument(s). This is why JobCallback() requires a Dialer as a parameter -- it does not know how to guess it either. Comm callback improvements will need a dedicated PR.

rousskov avatar Jul 19 '22 19:07 rousskov