You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Sahlberg <ds...@apache.org> on 2024/11/28 21:47:22 UTC
Re: Python's removal of asyncore/asynchat in 3.12 affects svnpubsub/client.py
On 2023/07/03 09:37:01 Yasuhito FUTATSUKI wrote:
> Hi,
>
> On 2023/07/03 3:17, James McCoy wrote:
> > Hi all,
> >
> > Python 3.6 deprecated the asyncore and asynchat modules in favor of the
> > asyncio module. Per [PEP 594], that means that the modules
> > will be removed in Python 3.12, thus breaking svnpubsub/client.py.
> >
> > Is there someone familiar enough with these Python modules to work on
> > updating svnpubsub? What's our policy on minimum supported Python
> > version for the tools?
>
> Before it, nobody check (and/or report) that the scripts in
> tools/server-side/svnpubsub can run with any version of Python 3.x.
>
> https://cwiki.apache.org/confluence/display/SVN/Subversion%27s+Python+3+Support+Status
>
> It seems that those tools in the section "Not Checked For Python 3 Yet"
> are now unmantained.
>
> > [PEP 594]: https://peps.python.org/pep-0594/#deprecated-modules
> >
> > Cheers,
>
> Cheers,
> --
> Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>
>
I took some time today to try to figure out how it all connects. I made a minimal client which I believe should be correct:
[[[
def cc(o):
print("commit " + repr(o))
def ec(url, type, o):
print("event on " + repr(url) + ": " + repr(type) + "\n" + repr(o))
def mc(o):
print("metadata " + repr(o))
import client
c=client.MultiClient(["http://pubsub.apache.org:2069"], cc, ec, mc)
c.run_forever()
]]]
Running on Python 3.11.2, I only get the following output:
[[[
event on 'http://pubsub.apache.org:2069': 'connected'
None
event on 'http://pubsub.apache.org:2069': 'closed'
None
event on 'http://pubsub.apache.org:2069': 'connected'
None
event on 'http://pubsub.apache.org:2069': 'closed'
None
...
]]]
'closed' come immediately after 'connected'. Between each pair there is approximately 30 seconds
I also tries settingup svnwcsub.py/svnwcsub.conf and I got it running but with the same events as above.
Running curl on the same endpoint print a "stillalive" event every 5 seconds (with the occational actual event inbetween):
[[[
$ curl http://pubsub.apache.org:2069
{"stillalive": 1732781004.3811345}
{"stillalive": 1732781009.3833125}
]]]
Comparing the svnpubsub and curl network traffic using tcpdump, I noted that svnpubsub use HTTP/1.0 while curl use HTTP/1.1. I also saw some differences between the format actual on-the-wire-format by pubsub.apache.org (Transfer-Encoding: chunked, so each record is a tuple consisting of [length] and [json] separated and terminated by \r\n) and the expected formt (record separator \0 and no length field).
Checking the code for pypubsub [1] it seems the \0 can be added depending on the User-Agent header but it seems to always use Transfer-Encoding: chunked.
I've tested the below changes:
[[[
Index: client.py
===================================================================
--- client.py (revision 1922187)
+++ client.py (working copy)
@@ -97,7 +97,8 @@
self.handle_error()
return
- self.push(('GET %s HTTP/1.0\r\n\r\n' % resource).encode('ascii'))
+ self.push(('GET %s HTTP/1.1\r\nHost: %s\r\n\r\n' %
+ (resource, host + ":" + str(port))).encode('ascii'))
def handle_connect(self):
self.event_callback('connected', None)
@@ -113,12 +114,20 @@
def found_terminator(self):
if self.skipping_headers:
self.skipping_headers = False
- # Each JSON record is terminated by a null character
- self.set_terminator(b'\0')
+ # pypubsub is using Transfer-Encoding: chunked so each message is
+ # 0xLength\r\n{json...}\n\r\n
+ # ^^^^^^^^^^ <- This is the Length
+ # Set terminator to the trailing \n plus the second \r\n to catch the
+ # whole message
+ self.set_terminator(b'\n\r\n')
else:
- record = b"".join(self.ibuffer)
+ record = b"".join(self.ibuffer).decode()
self.ibuffer = []
- self.parser.feed(record.decode())
+ # Cut out the length before parsing the json
+ index = record.find('\r\n')
+ if index:
+ record = record[index+2:]
+ self.parser.feed(record)
def collect_incoming_data(self, data):
# Remember the last time we saw activity
]]]
With this, the client seems to work. I'd appreciate if someone with Python knowledge can review it before I commit.
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
Re: Python's removal of asyncore/asynchat in 3.12 affects svnpubsub/client.py
Posted by Daniel Gruno <hu...@apache.org>.
pypubsub uses a slightly different protocol than svnpubsub, so you shouldn't really be testing against it (unless you want to support both protocols, in which case that'd be awesome - https://infra.apache.org/pypubsub.html can help you there).
The svnpubsub service we have is still located at svn.apache.org:2069.
On 2024/11/28 21:47:22 Daniel Sahlberg wrote:
> On 2023/07/03 09:37:01 Yasuhito FUTATSUKI wrote:
> > Hi,
> >
> > On 2023/07/03 3:17, James McCoy wrote:
> > > Hi all,
> > >
> > > Python 3.6 deprecated the asyncore and asynchat modules in favor of the
> > > asyncio module. Per [PEP 594], that means that the modules
> > > will be removed in Python 3.12, thus breaking svnpubsub/client.py.
> > >
> > > Is there someone familiar enough with these Python modules to work on
> > > updating svnpubsub? What's our policy on minimum supported Python
> > > version for the tools?
> >
> > Before it, nobody check (and/or report) that the scripts in
> > tools/server-side/svnpubsub can run with any version of Python 3.x.
> >
> > https://cwiki.apache.org/confluence/display/SVN/Subversion%27s+Python+3+Support+Status
> >
> > It seems that those tools in the section "Not Checked For Python 3 Yet"
> > are now unmantained.
> >
> > > [PEP 594]: https://peps.python.org/pep-0594/#deprecated-modules
> > >
> > > Cheers,
> >
> > Cheers,
> > --
> > Yasuhito FUTATSUKI <fu...@yf.bsdclub.org>
> >
>
> I took some time today to try to figure out how it all connects. I made a minimal client which I believe should be correct:
> [[[
> def cc(o):
> print("commit " + repr(o))
>
> def ec(url, type, o):
> print("event on " + repr(url) + ": " + repr(type) + "\n" + repr(o))
>
> def mc(o):
> print("metadata " + repr(o))
>
> import client
> c=client.MultiClient(["http://pubsub.apache.org:2069"], cc, ec, mc)
> c.run_forever()
> ]]]
>
> Running on Python 3.11.2, I only get the following output:
> [[[
> event on 'http://pubsub.apache.org:2069': 'connected'
> None
> event on 'http://pubsub.apache.org:2069': 'closed'
> None
> event on 'http://pubsub.apache.org:2069': 'connected'
> None
> event on 'http://pubsub.apache.org:2069': 'closed'
> None
> ...
> ]]]
> 'closed' come immediately after 'connected'. Between each pair there is approximately 30 seconds
>
> I also tries settingup svnwcsub.py/svnwcsub.conf and I got it running but with the same events as above.
>
> Running curl on the same endpoint print a "stillalive" event every 5 seconds (with the occational actual event inbetween):
> [[[
> $ curl http://pubsub.apache.org:2069
> {"stillalive": 1732781004.3811345}
> {"stillalive": 1732781009.3833125}
> ]]]
>
> Comparing the svnpubsub and curl network traffic using tcpdump, I noted that svnpubsub use HTTP/1.0 while curl use HTTP/1.1. I also saw some differences between the format actual on-the-wire-format by pubsub.apache.org (Transfer-Encoding: chunked, so each record is a tuple consisting of [length] and [json] separated and terminated by \r\n) and the expected formt (record separator \0 and no length field).
>
> Checking the code for pypubsub [1] it seems the \0 can be added depending on the User-Agent header but it seems to always use Transfer-Encoding: chunked.
>
> I've tested the below changes:
>
> [[[
> Index: client.py
> ===================================================================
> --- client.py (revision 1922187)
> +++ client.py (working copy)
> @@ -97,7 +97,8 @@
> self.handle_error()
> return
>
> - self.push(('GET %s HTTP/1.0\r\n\r\n' % resource).encode('ascii'))
> + self.push(('GET %s HTTP/1.1\r\nHost: %s\r\n\r\n' %
> + (resource, host + ":" + str(port))).encode('ascii'))
>
> def handle_connect(self):
> self.event_callback('connected', None)
> @@ -113,12 +114,20 @@
> def found_terminator(self):
> if self.skipping_headers:
> self.skipping_headers = False
> - # Each JSON record is terminated by a null character
> - self.set_terminator(b'\0')
> + # pypubsub is using Transfer-Encoding: chunked so each message is
> + # 0xLength\r\n{json...}\n\r\n
> + # ^^^^^^^^^^ <- This is the Length
> + # Set terminator to the trailing \n plus the second \r\n to catch the
> + # whole message
> + self.set_terminator(b'\n\r\n')
> else:
> - record = b"".join(self.ibuffer)
> + record = b"".join(self.ibuffer).decode()
> self.ibuffer = []
> - self.parser.feed(record.decode())
> + # Cut out the length before parsing the json
> + index = record.find('\r\n')
> + if index:
> + record = record[index+2:]
> + self.parser.feed(record)
>
> def collect_incoming_data(self, data):
> # Remember the last time we saw activity
> ]]]
>
> With this, the client seems to work. I'd appreciate if someone with Python knowledge can review it before I commit.
>
> 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
>
Re: Python's removal of asyncore/asynchat in 3.12 affects svnpubsub/client.py
Posted by Daniel Gruno <hu...@apache.org>.
On 12/31/24 02:31, Greg Stein wrote:
> 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.
As far as I recall, asfpy's pubsub works with both \n and \x00 endings
out of the box[1]. I suppose you could test it on our svnpubsub stream
and find out?
[1]
https://github.com/apache/infrastructure-asfpy/blob/main/asfpy/pubsub.py#L218
>
> Check out the pubsub.py module in https://github.com/apache/
> infrastructure-asfpy/ <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 <https://github.com/
> apache/infrastructure-pypubsub> (downstream of:)
> * https://github.com/Humbedooh/pypubsub <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.sahlberg@gmail.com <ma...@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 <jun66j5@gmail.com
> <ma...@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 <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/ <http://
> svn.apache.org:2069/commits/>': 'ping'
> 1734899014.037649
> event on 'http://svn.apache.org:2069/commits/ <http://
> svn.apache.org:2069/commits/>': 'ping'
> 1734899029.038485
> commit http://svn.apache.org:2069/commits/ <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/ <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
>
Re: Python's removal of asyncore/asynchat in 3.12 affects svnpubsub/client.py
Posted by Greg Stein <gs...@gmail.com>.
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 <da...@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 <ju...@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
>
>
Re: Python's removal of asyncore/asynchat in 3.12 affects svnpubsub/client.py
Posted by Daniel Sahlberg <da...@gmail.com>.
Thank you very much for the review! I'll answer inline below.
Den tis 3 dec. 2024 kl 22:31 skrev Jun Omae <ju...@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
Re: Python's removal of asyncore/asynchat in 3.12 affects svnpubsub/client.py
Posted by Jun Omae <ju...@gmail.com>.
Hi,
On 2024/11/29 6:47, Daniel Sahlberg wrote:
> I took some time today to try to figure out how it all connects. I made a minimal client which I believe should be correct:
> [[[
> def cc(o):
> print("commit " + repr(o))
>
> def ec(url, type, o):
> print("event on " + repr(url) + ": " + repr(type) + "\n" + repr(o))
>
> def mc(o):
> print("metadata " + repr(o))
>
> import client
> c=client.MultiClient(["http://pubsub.apache.org:2069"], cc, ec, mc)
> c.run_forever()
> ]]]
>
> Running on Python 3.11.2, I only get the following output:
> [[[
> event on 'http://pubsub.apache.org:2069': 'connected'
> None
> event on 'http://pubsub.apache.org:2069': 'closed'
> None
> event on 'http://pubsub.apache.org:2069': 'connected'
> None
> event on 'http://pubsub.apache.org:2069': 'closed'
> None
> ...
> ]]]
> 'closed' come immediately after 'connected'. Between each pair there is approximately 30 seconds
>
> I also tries settingup svnwcsub.py/svnwcsub.conf and I got it running but with the same events as above.
>
> Running curl on the same endpoint print a "stillalive" event every 5 seconds (with the occational actual event inbetween):
> [[[
> $ curl http://pubsub.apache.org:2069
> {"stillalive": 1732781004.3811345}
> {"stillalive": 1732781009.3833125}
> ]]]
>
> Comparing the svnpubsub and curl network traffic using tcpdump, I noted that svnpubsub use HTTP/1.0 while curl use HTTP/1.1. I also saw some differences between the format actual on-the-wire-format by pubsub.apache.org (Transfer-Encoding: chunked, so each record is a tuple consisting of [length] and [json] separated and terminated by \r\n) and the expected formt (record separator \0 and no length field).
>
> Checking the code for pypubsub [1] it seems the \0 can be added depending on the User-Agent header but it seems to always use Transfer-Encoding: chunked.
>
> I've tested the below changes:
>
> [[[
> Index: client.py
> ===================================================================
> --- client.py (revision 1922187)
> +++ client.py (working copy)
> @@ -97,7 +97,8 @@
> self.handle_error()
> return
>
> - self.push(('GET %s HTTP/1.0\r\n\r\n' % resource).encode('ascii'))
> + self.push(('GET %s HTTP/1.1\r\nHost: %s\r\n\r\n' %
> + (resource, host + ":" + str(port))).encode('ascii'))
>
> def handle_connect(self):
> self.event_callback('connected', None)
> @@ -113,12 +114,20 @@
> def found_terminator(self):
> if self.skipping_headers:
> self.skipping_headers = False
> - # Each JSON record is terminated by a null character
> - self.set_terminator(b'\0')
> + # pypubsub is using Transfer-Encoding: chunked so each message is
> + # 0xLength\r\n{json...}\n\r\n
> + # ^^^^^^^^^^ <- This is the Length
> + # Set terminator to the trailing \n plus the second \r\n to catch the
> + # whole message
> + self.set_terminator(b'\n\r\n')
> else:
> - record = b"".join(self.ibuffer)
> + record = b"".join(self.ibuffer).decode()
> self.ibuffer = []
> - self.parser.feed(record.decode())
> + # Cut out the length before parsing the json
> + index = record.find('\r\n')
> + if index:
> + record = record[index+2:]
> + self.parser.feed(record)
>
> def collect_incoming_data(self, data):
> # Remember the last time we saw activity
> ]]]
>
> With this, the client seems to work. I'd appreciate if someone with Python knowledge can review it before I commit.
>
> 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'))
]]]
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
]]]
--
Jun Omae <ju...@gmail.com> (大前 潤)