Incidencia #45207

json protocol issue

Abrir Fecha: 2022-07-24 22:31 Última actualización: 2022-07-26 17:22

5 - Medium
5 - Medium


It seems that something has gone wrong with hrm #745593

Freeciv-web isn't updated to a git revision with that commit yet, but when I apply it as a patch on top of a commit over with which it should work, server <-> web client communication breaks. I don't know when things have broken that way - presumably with the last rebase I did for the patch, though I think I did testing with that too once already.

Ticket History (3/14 Histories)

2022-07-24 22:31 Updated by: cazfi
  • New Ticket "json protocol issue" created
2022-07-24 22:40 Updated by: alienvalkyrie

Heads up: If this ends up requiring changes to, those will have to wait until after (the relevant part of) #45206 is done.

Also, if those changes have to do with an edge case in the non-delta versions of diff arrays (which it probably doesn't, realistically speaking), that'll get fixed along the way by itself (keeping the problem would require extra work).

2022-07-24 23:20 Updated by: cazfi
  • Componente Update from (Ninguno) to General

Reproducible with desktop server + client configured with "--enable-debug --enable-json --disable-delta-protocol"

No matter which direction you try to move units, they always move up (direction 0, I think)

You think this is something you could debug while working on & related stuff, alienvalkyrie?

2022-07-24 23:53 Updated by: alienvalkyrie

I'll take a look at it, tho unless it ends up being a sufficiently obvious error in the generation script, I'll likely have to take some time to familiarize myself with the dataio_json internals.

2022-07-25 01:50 Updated by: alienvalkyrie

The problem is actually larger - the entire unit_order struct is zeroed, not just the dir; it just so happens that (enum unit_orders)(0) is ORDER_MOVE, so the unit still moves, and that doesn't care about any of the other fields. Generated code looks clean, and so does dio_get_unit_order_json() ~> error lies deeper.

Likely culprit: plocation_read_elem() doesn't recurse properly. It does some sanity checks, and seems to recurse into nested arrays, but not into objects inside arrays – which is exactly the case in PACKET_UNIT_ORDERS. So we're trying to reinterpret the object as (in this case) an integer, and since json_integer_value() has no way of telling us about an error, we just silently get 0.

Note that the other dio_get_<struct>_json() functions (that are actually used), i.e. requirement, worklist, and action probability, all sidestep this problem by grabbing the object and manually reading from it using dio_get_*_internal(), rather than temporarily setting a sub_location on the location it gets and using public-facing dio_get_*() functions.

2022-07-25 02:03 Updated by: alienvalkyrie
  • Propietario Update from (Ninguno) to alienvalkyrie
  • Resolución Update from Ninguno to Accepted

Attached patch makes plocation_read_elem() function analogously to plocation_read_field() (i.e. rolls back that part); tested and it seems to fix the issue. It does get rid of the checks that were there before, but I'm not convinced those checks were really necessary / sensible – if they are, we should bring them back and mirror them in plocation_read_field().

(Edited, 2022-07-25 02:35 Updated by: alienvalkyrie)
2022-07-25 02:45 Updated by: cazfi

With this "--enable-debug --enable-json" (delta protocol not disabled) game does not launch at all: "1: ERROR: Unable to get uint8 from location: global_advances"

Without the patch it suffered from the same problem as the deltaless json once you are in the game.

2022-07-25 03:17 Updated by: alienvalkyrie

Reply To cazfi

With this "--enable-debug --enable-json" (delta protocol not disabled) game does not launch at all: "1: ERROR: Unable to get uint8 from location: global_advances" Without the patch it suffered from the same problem as the deltaless json once you are in the game.

That seems like it might be a different problem with writing diff-arrays. Do note that with --enable-json --disable-delta-protocol, the global_advances array will not have really been read at all – that's the edge case in non-delta versions of diff arrays that I mentioned before (which will be passively fixed by #45219).

Looks like JSON array-diff put produces [[index, value], [i,v], 255], when it should be [[index, value], [i, v], [255]], i.e. the terminating index 255 doesn't get wrapped in an array like it should. Since it seems like that was also introduced in hrm # 745593, should we fold that into this ticket as "fix regressions", or make it a separate one?

2022-07-25 03:29 Updated by: alienvalkyrie

Decided to go the "fold both into one patch" route. Still need to test that this didn't break the non-delta-protocol again.

2022-07-25 03:48 Updated by: alienvalkyrie

Both delta and non-delta appear to work correctly now (apart from the other issue I mentioned, which I've already fixed locally as part of #45219).

2022-07-26 17:22 Updated by: alienvalkyrie
  • Resolución Update from Accepted to Fixed
2022-07-26 17:22 Updated by: alienvalkyrie
  • Estado Update from Open to Cerrado


Please login to add comment to this ticket » Entrar