search

PeterJCLaw / srcomp-puppet

Student Robotics Competition Server
https://github.com/PeterJCLaw/srcomp/wiki/Component-Overview#srcomp-puppet
MIT License
0 stars 4 forks source link

Switch to using the pystream eventstream #24

Closed WillB97 closed 1 year ago

WillB97 commented 1 year ago

Moves to a python (aiohttp) based eventstream server (https://github.com/WillB97/srcomp-pystream) that doesn't suffer from a memory leak. Corrects the JSON encoding of unicode characters in team names.

I have checked this installs and run mixtape against it for a week without issues.

PeterJCLaw commented 1 year ago

Thanks for this contribution. I'm having a look at the pystream implementation now -- has that been reviewed at all, or does that need doing as part of this change?

Corrects the JSON encoding of unicode characters in team names.

Please could you expand on what this issue is? I don't recall this being an issue and I can't find any bug reports which mention it.

I have checked this installs and run mixtape against it for a week without issues.

Did that run include deploys and matches or was it just running against a constant state? The events emitted while nothing is scheduled are quite different than those emitted during a competition event.

It would also be useful to understand what testing has been done to ensure compatibility with the existing stream, unless you're planning also to update all the various clients?

WillB97 commented 1 year ago

The current stream leaves raw unicode characters in the JSON output, which is out of spec. This converts them to \u escaped values. Javascript and python's JSON implementations seem to cope with this so it's not a major issue but something that may crop up in other implementations.

In testing I ran it against ~2000 matches, deploying scores, changing the schedule and adding delay. For compatibility I verified the output between this and the previous stream for 200 matches across the same as above.

PeterJCLaw commented 1 year ago

The current stream leaves raw unicode characters in the JSON output, which is out of spec.

Do you have a reference for that? Looking at the spec on https://www.json.org it says:

A string is a sequence of zero or more Unicode characters, wrapped in double quotes, using backslash escapes.

The image also says (in the box between the quotes):

Any codepoint except " or \ or control characters

My reading of this is that unicode characters can be directly part of the quoted string. If higher valued codepoints needed converting to a \u.... then it would say that (or mention that the quoted values needed to be ASCII or something).

Converting them to \u.... is therefore still valid, however definitely not required.

PeterJCLaw commented 1 year ago

In testing I ran it against ~2000 matches, deploying scores, changing the schedule and adding delay. For compatibility I verified the output between this and the previous stream for 200 matches across the same as above.

Nice, that sounds like a good round of tests.

I've looked through the pystream implementation, with a focus on the event construction. I've not delved into the websocket stuff as that's not an area I'm familiar with unfortunately. It would be great to have someone who knows websockets review that at some point. I've done some local testing too and it does seem to (mostly) work as a drop-in replacement for the existing stream.

I've raised some issues & PRs against the pystream repo in the course of reviewing it. Many of them are small things, though I think some of them we should aim to address before moving over:

If possible it would be great to address https://github.com/WillB97/srcomp-pystream/issues/5 too, hopefully it's a small change, though that probably isn't a blocker to moving over.

PeterJCLaw commented 1 year ago

Thanks! Let's see how this goes tomorrow :)

I realised we also needed to put the pystream in its own folder, so I added that.