Maintenance: move async and events code to libasync #1635

kinkie · 2024-01-13T19:14:47Z

Move Async and Events related code
to own library in src/async

async/forward.h needs to include base/forward.h to avoid duplicating the declaration of RefCount

kinkie · 2024-01-13T19:15:37Z

Note: I have intentionally avoided creating Async namespace, this PR touches so much code already.
If there is consensus to do it, I'll do that as a followup PR

kinkie · 2024-01-13T19:16:40Z

The idea for this PR stems from a comment in PR #1548

rousskov · 2024-01-13T21:51:13Z

I have intentionally avoided creating Async namespace, this PR touches so much code already.

FWIW, I do not think creating Async namespace is a good idea (in any PR).

yadij · 2024-01-13T22:51:36Z

I have intentionally avoided creating Async namespace, this PR touches so much code already.

FWIW, I do not think creating Async namespace is a good idea (in any PR).

And yet it was you who decided all these objects should consume the Async prefix (a.k.a. namespace).
You have a better alternative to rename them as?

IMO the AsyncJob and related pieces are sufficiently complex and self-contained to deserve their own library+namespace like this.

However, @kinkie please do not merge the EventLoop mechanism into it. That should have a separate library of its own.

yadij · 2024-01-13T22:53:56Z

@kinkie, also be aware the AsyncEngine is an interface for the Engine mechanism, not the part of AsyncJob mechanism.

kinkie · 2024-01-14T10:37:38Z

I have intentionally avoided creating Async namespace, this PR touches so much code already.

FWIW, I do not think creating Async namespace is a good idea (in any PR).

Sure, but could you highlight why? It would be a deviation from standard practice, isn't it?

This reverts commit c7a705e.

yadij · 2024-01-16T17:48:13Z

src/AsyncEngine.cc

@@ -8,4 +8,3 @@

 #include "squid.h"
 #include "AsyncEngine.h"
-


File should no longer be touched.

yadij · 2024-01-16T17:51:40Z

src/FwdState.cc

@@ -27,7 +28,6 @@
 #include "comm/Loops.h"
 #include "CommCalls.h"
 #include "errorpage.h"
-#include "event.h"


This event.h and corresponding event.cc code is all part of the EventLoop mechanism. Not part of AsyncJob code, even though the two are similar and interact.

yadij · 2024-01-16T17:52:20Z

src/Makefile.am


 if ENABLE_AUTH
 SUBDIRS += auth
 AUTH_LIBS= auth/libauth.la
 AUTH_ACL_LIBS= auth/libacls.la
 endif

-SUBDIRS	+= http ip icmp ident log ipc mgr
+SUBDIRS	+=  http ip icmp ident log ipc mgr


typo

Suggested change

SUBDIRS += http ip icmp ident log ipc mgr

SUBDIRS += http ip icmp ident log ipc mgr

yadij · 2024-01-16T17:54:34Z

src/async/forward.h

+class CallDialer;
+class DelayedAsyncCalls;
+
+template <class Answer> class AsyncCallback;


Suggested change

template <class Answer> class AsyncCallback;

template<class Answer> class AsyncCallback;

yadij · 2024-01-16T17:55:16Z

src/async/forward.h

+template<class Job> class JobWait;
+
+template<class Cbc> class CbcPointer;
+typedef CbcPointer<AsyncJob> AsyncJobPointer;


Suggested change

typedef CbcPointer<AsyncJob> AsyncJobPointer;

using AsyncJobPointer = CbcPointer<AsyncJob>;

rousskov

Alex: FWIW, I do not think creating Async namespace is a good idea (in any PR).

Francesco: Sure, but could you highlight why?

I am not convinced that every Squid class must be in a namespace¹. If some Squid classes should not be in a namespace, then it is reasonable to question whether, say, AsyncCall should be. To me, AsyncCall feels similar to SBuf -- both do not really need a dedicated namespace¹ AFAICT.

I failed to find an applicable industry guideline regarding the use of namespaces in similar contexts.

The value of numerous users using "Async::Call" (instead of existing "AsyncCall") just so that AsyncCall implementation can simply say "Call" is rather questionable.

If all deferred calls (whatever we decide that or a similar term means) should migrate to (the variation of) AsyncCall API, then we may even decide to drop "Async" itself. One large change is better than two (with the same end result) in this case.

I will change my position if somebody proves that we should change 800+ lines of code to migrate AsyncCall and AsyncJob to Async namespace, but I believe that change proposal must come first, before I am forced to spend even more time on discussing a change that has not been really proposed yet.

Francesco: It would be a deviation from standard practice, isn't it?

You will need to define "standard practice" for me to be able to answer that question. I can interpret that term many different ways. Needless to say, Squid does not use namespaces for some code grouped inside src directories, and I am not aware of any requirement to use a namespace for all src directories code. Each directory or group of related names deserves a dedicated decision IMO.

Amos: And yet it was you who decided all these objects should consume the Async prefix (a.k.a. namespace).

I have been begging to stop posting reviews of people and yet here we go again.

Squid classes used "Async prefix" before "all these objects" were introduced
a class name prefix is not the same as a C++ namespace
not "all these objects" consume the "Async prefix"

Amos: You have a better alternative to rename them as?

I am not convinced they should be renamed at this time.

Amos: IMO the AsyncJob and related pieces are sufficiently complex and self-contained to deserve their own library+namespace like this.

FWIW, I am (still) OK with grouping AsyncCall-related code in a dedicated directory and convenience library. The devil is in the details (e.g., what code to move, what directory name to use, whether to introduce a new namespace, whether to rename anything, etc.). For example, we can move without introducing a new namespace. I believe those details are being hammered out elsewhere, and this PR should continue to wait for the conclusion of those discussions.

Amos: the EventLoop mechanism ... should have a separate library of its own.

FWIW, there is currently no consensus on whether EventLoop deserves a library of its own.

Other than the existing default global namespace and "Squid" namespace that we might use more aggressively for reasons unrelated to this discussion. ↩ ↩²

kinkie added 10 commits January 13, 2024 11:35

Move async code to async/libasync.la

7a3db60

Move many Async files from base/ to async/

965804c

adjust Makefile.am

594873c

add async/forward.h

4dc0fc9

move DelayedAsyncCalls to async

f0f5f04

move JobWait to async/

f0a7c43

move Subscription.h to async

218faef

Adjust forwards

2f02a25

async/forward.h needs to include base/forward.h to avoid duplicating the declaration of RefCount

event.h -> async/Event.h

e6a5240

move EventLoop to async/

c7a705e

kinkie changed the title ~~Create libasync~~ Maintenance: move async and events code to async/ Jan 13, 2024

kinkie changed the title ~~Maintenance: move async and events code to async/~~ Maintenance: move async and events code to libasync.la Jan 13, 2024

kinkie changed the title ~~Maintenance: move async and events code to libasync.la~~ Maintenance: move async and events code to libasync Jan 13, 2024

Source maintenance

916de6e

kinkie marked this pull request as draft January 13, 2024 22:03

kinkie changed the title ~~Maintenance: move async and events code to libasync~~ [WIP] Maintenance: move async and events code to libasync Jan 13, 2024

fix header in DelayIdComposite.h

becb4aa

Revert "move EventLoop to async/"

1f422bb

This reverts commit c7a705e.

rousskov self-requested a review January 14, 2024 14:15

kinkie added 2 commits January 14, 2024 16:33

Revert moving AsyncEngine

8bffd02

Source maintenance

bd8de57

kinkie changed the title ~~[WIP] Maintenance: move async and events code to libasync~~ Maintenance: move async and events code to libasync Jan 14, 2024

kinkie marked this pull request as ready for review January 14, 2024 16:37

yadij requested changes Jan 16, 2024

View reviewed changes

rousskov requested changes Jan 18, 2024

View reviewed changes

kinkie marked this pull request as draft February 1, 2024 12:01

kinkie added the M-ignored-by-merge-bots https://github.com/measurement-factory/anubis/blob/master/README.md#pull-request-labels label Mar 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Maintenance: move async and events code to libasync #1635

Maintenance: move async and events code to libasync #1635

Uh oh!

kinkie commented Jan 13, 2024

Uh oh!

kinkie commented Jan 13, 2024

Uh oh!

kinkie commented Jan 13, 2024

Uh oh!

rousskov commented Jan 13, 2024 •

edited

Loading

Uh oh!

yadij commented Jan 13, 2024

Uh oh!

yadij commented Jan 13, 2024

Uh oh!

kinkie commented Jan 14, 2024

Uh oh!

yadij Jan 16, 2024

Uh oh!

yadij Jan 16, 2024

Uh oh!

yadij Jan 16, 2024

Uh oh!

yadij Jan 16, 2024

Uh oh!

yadij Jan 16, 2024

Uh oh!

rousskov left a comment

Uh oh!

Uh oh!

	SUBDIRS += http ip icmp ident log ipc mgr
	SUBDIRS += http ip icmp ident log ipc mgr

	template <class Answer> class AsyncCallback;
	template<class Answer> class AsyncCallback;

	typedef CbcPointer<AsyncJob> AsyncJobPointer;
	using AsyncJobPointer = CbcPointer<AsyncJob>;

Maintenance: move async and events code to libasync #1635

Are you sure you want to change the base?

Maintenance: move async and events code to libasync #1635

Uh oh!

Conversation

kinkie commented Jan 13, 2024

Uh oh!

kinkie commented Jan 13, 2024

Uh oh!

kinkie commented Jan 13, 2024

Uh oh!

rousskov commented Jan 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yadij commented Jan 13, 2024

Uh oh!

yadij commented Jan 13, 2024

Uh oh!

kinkie commented Jan 14, 2024

Uh oh!

yadij Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

yadij Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

yadij Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

yadij Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

yadij Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

rousskov left a comment

Choose a reason for hiding this comment

Footnotes

Uh oh!

Uh oh!

rousskov commented Jan 13, 2024 •

edited

Loading