Skip to content

Initial support for engines library#2367

Open
yadij wants to merge 6 commits intosquid-cache:masterfrom
yadij:arc-engines-1
Open

Initial support for engines library#2367
yadij wants to merge 6 commits intosquid-cache:masterfrom
yadij:arc-engines-1

Conversation

@yadij
Copy link
Contributor

@yadij yadij commented Feb 4, 2026

Squid main operational loop uses a set of Engines that perform
various roles.

Create a library for the code underlying the engines logic.

@yadij
Copy link
Contributor Author

yadij commented Feb 4, 2026

This PR is an evolution of the previous attempts in PR #1548, with input from discussions in PR #1635.

The consensus AFAICT is that we should have a library for this logic but not a specific namespace. So this PR sets up the library using the model previously used by sr/clients/ and src/servers/. Where the individual engines may have their own namespaces, but the libengines.la supplies a shared hierarchy API class (this AsyncEngine) and perhaps the MainLoop/EventLoop code in the global namespace.

Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I plan to come back to this PR after the backlog is cleared.

#include "adaptation/ecap/Host.h"
#include "adaptation/ecap/ServiceRep.h"
#include "adaptation/ecap/XactionRep.h"
#include "AsyncEngine.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not remove a header declaring a class that is used by the including code, even if some other included header happens to include that class declaration today. The same concern probably applies to some other modified files as well.


include $(top_srcdir)/src/Common.am

noinst_LTLIBRARIES = libengines.la
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR description: Squid main operational loop uses a set of Engines that perform various roles. This creates a library for the code underlying that logic.

Amos: This PR is an evolution of the previous attempts in PR #1548, with input from discussions in PR #1635. The consensus AFAICT is that we should have a library for this logic but not a specific namespace.

It is not clear to me what "underlying that logic" and "this logic" refers to here, so I cannot confirm that such a consensus exists. Creating a library consisting of one header file feels wrong, so I assume that this PR implies that some other code (containing some additional "logic") will be added to that library. That "other code" will help flesh out the actual library scope. We should decide what else should go into that library before finalizing and merging this PR. We do not need to agree on a comprehensive list, but we need to agree on more than just one header file. The comment quoted below may be a step towards making that decision:

Amos: the libengines.la supplies a shared hierarchy API class (this AsyncEngine) and perhaps the MainLoop/EventLoop code

Grouping AsyncEngine API and EventLoop code together makes sense, especially since that code is used by multiple executables today, but that group/library should not be called "engines" -- it provides no engines! A name like "MainLoop" or "EventLoop" may work (and would not preclude adding more engines to that library if we decide that those additions are a good idea).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR description: Squid main operational loop uses a set of Engines that perform various roles. This creates a library for the code underlying that logic.

Amos: This PR is an evolution of the previous attempts in PR #1548, with input from discussions in PR #1635. The consensus AFAICT is that we should have a library for this logic but not a specific namespace.

It is not clear to me what "underlying that logic" and "this logic" refers to here,

Updated the PR description.

FYI; abuse of a code change request to require changes to the PR description. Please use the review main comment for this type of thing.

Amos: the libengines.la supplies a shared hierarchy API class (this AsyncEngine) and perhaps the MainLoop/EventLoop code

Grouping AsyncEngine API and EventLoop code together makes sense, especially since that code is used by multiple executables today, but that group/library should not be called "engines" -- it provides no engines! A name like "MainLoop" or "EventLoop" may work (and would not preclude adding more engines to that library if we decide that those additions are a good idea).

As mentioned in my prior comment this PR is the basic infrastructure of the library. The other classes which inherit from AsyncEngine will also move in later in this refactor when their boundaries are cleaned up. Those are the actual "engines" it is named for providing (SignalEngine, TimeEngine, EventsEngine, ...) - same layout model as we use for HttpFooServer and FtpFooServer in libservers library, and equivalent in clients.

FTR; the model here is your preferred layout design. My own preferred layout is to separate each of these things by protocol. eg Http1Server and Http1Client would both be in src/http/ and TimeEngine would be in src/time/ and the trailing s on the library names would not be there. Anyway that ship has sailed years ago so I am just going with the model that has least resistance for this PR.

@@ -9,14 +9,13 @@
#ifndef SQUID_SRC_EVENTLOOP_H
#define SQUID_SRC_EVENTLOOP_H

#include "engines/AsyncEngine.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not pull class declarations where a forward declaration would suffice, especially when existing code already uses a forward declaration (that current PR branch removes below). Instead, please add a forward.h file for the new library and start accumulating forward declarations there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forward declaration does not suffice build by any class including EventLoop.h. This is code de-duplication and avoidance of the dependency issue which is well-known to effect BSD system headers (simply including header X requires prior include if header Y).

@yadij yadij requested a review from rousskov February 6, 2026 04:03
@yadij yadij added the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label Feb 6, 2026
yadij added 3 commits February 6, 2026 17:07
... and polish documentation
* use C++11 initialization for class members
* inline trivial class methods
* update unit test stub file
* update const correctness in API methods
* use ranged loops (removes need for typedef)
* wrap EVENT_LOOP_TIMEOUT to allow user customization

Logic changes tested with curl HTTP fetch and logging.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants