My recommendation would be to patch the asfpy package to understand the svnpubsub protocol (basically: \0 instead of \n). The asfpy.pubsub package is what Infra uses for all of its clients to listen to the official ASF pubsub service. It's fully async.
Check out the pubsub.py module in https://github.com/apache/infrastructure-asfpy/ Once patched for "\0", then we should point people there for a pubsub client. "asfpy" is a public package, maintained by the Foundation, and very willing to accept patches. (and delete our own client; shifting the server to asyncio is another beast; I'd recommend pointing people to pypubsub at one of: * https://github.com/apache/infrastructure-pypubsub (downstream of:) * https://github.com/Humbedooh/pypubsub It looks like some patches in the infra fork have not been merged upstream. Cheers, -g On Sun, Dec 22, 2024 at 2:31 PM Daniel Sahlberg <daniel.l.sahlb...@gmail.com> wrote: > Thank you very much for the review! I'll answer inline below. > > Den tis 3 dec. 2024 kl 22:31 skrev Jun Omae <jun6...@gmail.com>: > >> Hi, >> >> On 2024/11/29 6:47, Daniel Sahlberg wrote: >> > > [snip] > > >> > >> > The next step will be to rewrite for asyncio, but I'd like to have a >> functional client as a baseline. >> > >> > Cheers, >> > Daniel >> > >> > [1] >> https://github.com/apache/infrastructure-pypubsub/blob/master/pypubsub.py >> >> I'm not familiar with svnpubsub.py but tried to run the script with the >> proposed patch. >> >> However it doesn't work. Investigating it, 400 Bad Request is received >> because >> the client.py sends "GET HTTP/1.1" (missing request uri). After applying >> the >> following patch, it seems to work. >> >> [[[ >> diff --git a/tools/server-side/svnpubsub/svnpubsub/client.py >> b/tools/server-side/svnpubsub/svnpubsub/client.py >> index 871a5e925..004003512 100644 >> --- a/tools/server-side/svnpubsub/svnpubsub/client.py >> +++ b/tools/server-side/svnpubsub/svnpubsub/client.py >> @@ -75,7 +75,7 @@ class Client(asynchat.async_chat): >> raise ValueError("URL scheme must be http: '%s'" % url) >> host = parsed_url.hostname >> port = parsed_url.port >> - resource = parsed_url.path >> + resource = parsed_url.path or '/' >> if parsed_url.query: >> resource += "?%s" % parsed_url.query >> if parsed_url.fragment: >> ]]] >> >> I think it would simplify by the following rather than `host + ":" + >> str(port)`. >> [[[ >> - self.push(('GET %s HTTP/1.0\r\n\r\n' % resource).encode('ascii')) >> + self.push(('GET %s HTTP/1.1\r\nHost: %s:%d\r\n\r\n' % >> + (resource, host, port)).encode('ascii')) >> ]]] >> > > As Daniel Gruno noted, I was querying the wrong endpoint, so the HTTP/1.1 > protocol isn't really needed. > > We might want to support both protocols at some point in time, but I'll > leave these two enhancements for the future. > > >> >> In the minimal client, it uses `repr` but `__repr__` method of >> `Notification` >> class is not defined. I'd suggest to add the `__repr__` method. >> [[[ >> @@ -131,6 +140,8 @@ class Client(asynchat.async_chat): >> class Notification(object): >> def __init__(self, data): >> self.__dict__.update(data) >> + def __repr__(self): >> + return '%s(%r)' % (self.KIND, self.__dict__) >> >> class Commit(Notification): >> KIND = 'COMMIT' >> ]]] >> >> Finally, `cc` and `mc` methods in the minimal client should have `url` >> parameter. >> [[[ >> --- svnpubsub-client.py.orig 2024-12-04 06:25:33.083708157 +0900 >> +++ svnpubsub-client.py 2024-12-02 10:28:30.489176287 +0900 >> @@ -1,10 +1,10 @@ >> -def cc(o): >> +def cc(url, o): >> print("commit " + repr(o)) >> >> def ec(url, type, o): >> print("event on " + repr(url) + ": " + repr(type) + "\n" + repr(o)) >> >> -def mc(o): >> +def mc(url, o): >> print("metadata " + repr(o)) >> >> import client >> ]]] >> > > Good catch! With these two fixes, I now have a functioning client: > > [[[ > event on 'http://svn.apache.org:2069/commits/': 'ping' > 1734899014.037649 > event on 'http://svn.apache.org:2069/commits/': 'ping' > 1734899029.038485 > commit http://svn.apache.org:2069/commits/: COMMIT({'committer': > 'dsahlberg', 'log': 'Add a __repr__ function to allow repr() a Notification > (used for a debugging client).\n\n* > tools/server-side/svnpubsub/svnpubsub/client.py\n (Notifications): Add > __repr__\n\nSuggested by: jun66j5', 'repository': > '13f79535-47bb-0310-9956-ffa450edef68', 'format': 1, 'changed': > {'subversion/trunk/tools/server-side/svnpubsub/svnpubsub/client.py': > {'flags': 'U '}}, 'date': '2024-12-22 20:23:53 +0000 (Sun, 22 Dec 2024)', > 'type': 'svn', 'id': 1922639}) > event on 'http://svn.apache.org:2069/commits/': 'ping' > 1734899044.039507 > ]]] > > (The above is the client catching the commit of the __repr__ function). > > I'll continue with asyncio shortly. > > Cheers, > Daniel > >
