json persistent workers GA / --experimental_worker_allow_json_protocol removal

[keith]

JSON support for persistent workers launched a few months ago https://blog.bazel.build/2020/11/11/json-workers.html I'm curious what the criteria are for --experimental_worker_allow_json_protocol to no longer be required so we can use this in rules without having to force that flag on users?

[aiuto]

/cc: @susinmotion

[susinmotion]

cc: @larsrc-google
There are some changes to the worker protocol in progress, and we wanted to wait for those to be in place before flipping, then removing, the flag. Since we're not currently enforcing that JSON workers be tolerant of unknown fields in the work request, we were afraid we'd break JSON workers by changing the protocol.

Other things we considered were adding an arbitrary field to the work request to enforce resilience to unknown fields, then flipping the flag. We can still do this if there's good reason.

[meisterT]

I assume the JSON protocol is basically just push a dict over the wire. Unknown keys in the dict should not hurt any worker implementation, unless they specifically check for it.

[susinmotion]

Our own test workers weren't resilient to extra fields! :P

I'm comfortable flipping the flag but I defer to Lars's sense of risk.

[larsrc-google]

Our own workers aren't actually quite resilient yet, AFAICT.

[keith]

Is the thought that at the time of flipping this flag we would never again add fields though? based on the docs I believe that workers should be resilient to this and therefore that doesn't particularly have to affect this flip

[larsrc-google]

Oh, there will be more fields. Sandboxing for multiplex workers will require another field. And who knows what the future will bring? I definitely don't want to tie us to a frozen protocol.

[aiuto]

I'm going to raise Keith one. Workers *must* gracefully handle fields they don't understand. That should be a requirement for any code in any project that reads serialized data.

…

On Thu, Jun 24, 2021 at 12:35 PM larsrc-google ***@***.***> wrote: Oh, there will be more fields. Sandboxing for multiplex workers will require another field. And who knows what the future will bring? I definitely don't want to tie us to a frozen protocol. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#13599 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXHHHDOYNFFR63ZFSK4OF3TUNNFJANCNFSM47GZE7PQ> .

[larsrc-google]

Yes, we should be specific about that in the worker docs. We aren't quite.

[keith]

#13606

[keith]

I submitted #13607 in case we decide we're ready for this

[wolfd]

One thing I would love to change about the current JSON workers (before they're considered stable) is the lack of a newline between work requests. I found it fairly cumbersome to write a python worker without this. Since the JSON is not pretty-printed the only newlines in the stream should just be between work requests, which would allow looping and using .readline() on the file to get a complete request.

Right now this was the way I got a JSON worker working in Python:
https://github.com/wolfd/json_worker_example/blob/8238b21e623eea2b73174e927ab6601d5504cf57/concat.py#L8-L31

I decided not to change from Proto to JSON for a worker I have due to this problem. The Proto workers have the varint length of the request at the beginning of every request which makes it easier to delimit them.

[keith]

+1, I had the same problem but it's much worse in C++ https://github.com/bazelbuild/rules_swift/pull/657/files#diff-3897cb43ae588be2d9a1916486c372cb601c7391efb43e17462a323a4296d95eR85

[larsrc-google]

Hand-writing a JSON parser in C++ for this seems like a recipe for subtle errors. And I could imagine some line reading implementations that have limits on the line length shorter than we may end up needing.

[keith]

This isn't hand writing the parser, just the logic for reading the input from stdin

[aiuto]

All text protocols should terminate each individually parseable block with a newline on transmission, and accept any number of newlines as whitespace on reading. That is simply internet protocol best practice. OTOH in JSON you must be able to receive pretty printed input, so a simple readline is not sufficient either. You have to do multiple reads until you get a complete expression. Can someone file a bug for the newline as a reminder that we should fix this.

…

On Thu, Jun 24, 2021, 4:32 PM Keith Smiley ***@***.***> wrote: This isn't hand writing the parser, just the logic for reading the input from stdin — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#13599 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXHHHG7EGTI4FESYCPM2KDTUOI4XANCNFSM47GZE7PQ> .

[meisterT]

I thought we would use NDJSON to write those messages out. If not, my opinion is that we should in the future do so and describe it in the docs.

[aiuto]

What is NDJSON?

…

On Fri, Jun 25, 2021 at 2:19 AM Tobias Werth ***@***.***> wrote: I thought we would use NDJSON to write those messages out. If not, my opinion is that we should in the future do so and describe it in the docs. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#13599 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXHHHAXVSKCZH2E3AIQ3MLTUQNV3ANCNFSM47GZE7PQ> .

[larsrc-google]

Newline Delimited JSON. A very small standard:)

[meisterT]

See http://ndjson.org/

[ittaiz]

Hi,
I have a related question- I'm considering using json for a new persistent worker I need to write and was a bit concerned by the need to turn on experimental workers. There was a time in bazel (still?) that the statement was "your prod builds shouldn't have experimental flags turned on".
After reading this thread it sounds like the experimental is not about "if" json will stay or not but rather the "how" of the worker might still change and as a breaking change to me (a worker implementor).
Is that right? Thanks in advance!

[larsrc-google]

Yes, that is correct.

[keith]

#14219 should solve one of the major concerns about the format here

[keith]

I submitted #14679 for the flag cleanup now that it has been flipped elsewhere.