Project

Profile

Help

HostedRedmine.com has moved to the Planio platform. All logins and passwords remained the same. All users will be able to login and use Redmine just as before. Read more...

Bug #745593

JSON protocol: delta doesn't work

Added by Louis Moureaux over 4 years ago. Updated 25 days ago.

Status:
Closed
Priority:
Normal
Category:
General
Sprint/Milestone:
Start date:
Due date:
% Done:

0%

Estimated time:

Description

When configured with --enable-json, the clients can't connect to the server. It works with delta disabled (--enable-json --disable-delta-protocol).

Seen on master, but other branches are probably affected too.

fw_json_array_diffs_S3_1_200716_000.patch (27.8 KB) fw_json_array_diffs_S3_1_200716_000.patch patch for json array diffs RTX. christian montanari, 2020-07-17 09:39 PM
fw_json_array_diffs_S3_1_200716_001.patch (28.5 KB) fw_json_array_diffs_S3_1_200716_001.patch patch for json array diffs support on fc_S3_1. runs up to start game. christian montanari, 2020-07-18 05:17 AM
fw_json_array_diffs_S3_1_200716_002.patch (28.8 KB) fw_json_array_diffs_S3_1_200716_002.patch patch for json array diffs support on fc_S3_1. fixed diff-arrays elements construct christian montanari, 2020-07-18 08:22 AM
DeltaRebase_745593.patch (31.2 KB) DeltaRebase_745593.patch Marko Lindqvist, 2022-05-27 07:04 PM
DeltaRebase-220609_745593.patch (30.4 KB) DeltaRebase-220609_745593.patch Marko Lindqvist, 2022-06-09 03:51 AM
DeltaRebase-220611_745593.patch (19 KB) DeltaRebase-220611_745593.patch Marko Lindqvist, 2022-06-11 10:15 PM
0046-Fix-json-delta-protocol-combination.patch (6.01 KB) 0046-Fix-json-delta-protocol-combination.patch Marko Lindqvist, 2022-07-15 02:41 AM

Related issues

Blocks Freeciv - Bug #881773: the packet string parser is not adapted to JSON stringsNew2020-07-19

<a title="Actions" class="icon-only icon-actions js-contextmenu" href="#">Actions</a>

History

#1 Updated by christian montanari about 2 years ago

have this patch for S3_1 which takes care of the end token properly transmitted and retrieved.
and redundant sub_location field:

json: generate_packet: diff arrays are inserting the end token properly now.
  • . use of FC_FREE() macro
  • . beginning of implenting a flag error to check on any "put" actions.
  • . struct plocation.number set to size_t what is used as indexes in json_array sometimes!
  • . plocation_read_elem() handles redundant sub_locations
  • . more error detection and sanity checks in the get json primitives.

#2 Updated by christian montanari about 2 years ago

addendum to the patch...
  • break is missing in switch case of plocation_write_data.
  • also are static functions thread safe? I never know.

#3 Updated by christian montanari about 2 years ago

and finally...construction of the diff-arrays elements are working as should be.

#4 Updated by christian montanari about 2 years ago

and to completly fix the json parser... one has to add this patch...
to allow json-(hash) objects being embeded....

----------------------- common/networking/dataio_json.c -----------------------
index 2d5b0435e5..8ccdfd15be 100644
@@ -196,11 +196,11 @@ static json_t *plocation_read_elem(json_t *item,
           );
   }

   json_t *sub_item = json_array_get(item, location->number);
   if (location->sub_location != NULL) {
-    if (json_is_array(sub_item)) {
+    if (json_is_array(sub_item) || json_is_object(sub_item)) {
       //TODO::PTZ200717 not good... location->name looses its meaning here...
       // as could be calling another plocation_read_field....
       return plocation_read_data(sub_item, location->sub_location);
     } else if (sub_item == NULL) {
       //TODO::PTZ200717 this could be an error wth too many sub_location ....

but looking at the way this json, I wonder if we do not miss the point in this implementation for "packet" handlng.... json has its own parser and hash handling capablity... which are ignored here. so what the point ?

#5 Updated by Marko Lindqvist almost 2 years ago

Louis: Can you test if Christian's patches help?

#6 Updated by Marko Lindqvist almost 2 years ago

  • Assignee set to Louis Moureaux

#7 Updated by Louis Moureaux over 1 year ago

  • Assignee deleted (Louis Moureaux)

#8 Updated by Marko Lindqvist 4 months ago

  • Category set to General
  • Sprint/Milestone set to 3.2.0

While Louis no longer had use for the fix, it would be good to have the issue resolved for potential future cases. I don't think stable branches have particular use for this (as freeciv-web follows master anyway), so targeting to master only.

#9 Updated by Marko Lindqvist 4 months ago

christian montanari wrote:

I wonder if we do not miss the point in this implementation for "packet" handlng.... json has its own parser and hash handling capablity... which are ignored here. so what the point ?

Quite possible. All this has grown from separating json protocol from the original raw protocol, by implementing equivalent parts.

#10 Updated by Marko Lindqvist 4 months ago

Marko Lindqvist wrote:

targeting to master only.

Also helps with the style issue that "//" -commens are used in this C-code. CodingStyle has allowed that only since 3.2 development begun, i.e., currently only in master branch.

#11 Updated by Marko Lindqvist 4 months ago

christian montanari wrote:

addendum to the patch...

This actually did not apply over the earlier one (nor did the third path apply on top of this), but applies cleanly alone, so I guess the final patch is meant to be used alone? That's better than what I expected, a bit mislead by the word "addendum".

First problem encountered is that raw (default) protocol build does not compile. I've not even configured json-build yet.
A lot of "void value not ignored as it ought to be" errors about dio_put_..._raw() calls wrapped by DIO_PUT()

I'm going a bit backwards compared to normal process here, as I were trying to build before reviewing the code (thinking that there's no point in reviewing any parts of the code that will need rewrite even to build, after rebasing almost 2 years old patch to current master)

#12 Updated by Marko Lindqvist 4 months ago

  • Blocks Bug #881773: the packet string parser is not adapted to JSON strings added

#13 Updated by Marko Lindqvist 4 months ago

Some systematic style issues (check doc/CodingStyle)

- Empty line missing between variable declarations and actual code
- Where function return type has changed from "void" (4 characters) to "int" (3 characters), indentation of the following parameter lines has not been updated accordingly
- Some trailing spaces (At least for me 'git diff' shows these with glaring red)
- There was some commented-out code being added

#14 Updated by Marko Lindqvist 4 months ago

Marko Lindqvist wrote:

First problem encountered is that raw (default) protocol build does not compile. I've not even configured json-build yet.
A lot of "void value not ignored as it ought to be" errors about dio_put_..._raw() calls wrapped by DIO_PUT()

That's to be resolved in https://osdn.net/projects/freeciv/ticket/44458 as a dependency of this ticket.

#15 Updated by Marko Lindqvist 3 months ago

Patch was broken by recent generate_packets.py changes. Here it's rebased. Did also some style fixes while touching it.

#16 Updated by Marko Lindqvist 3 months ago

christian montanari wrote:

  • . struct plocation.number set to size_t what is used as indexes in json_array sometimes!

Split to -> https://osdn.net/projects/freeciv/ticket/44693

#17 Updated by Marko Lindqvist 2 months ago

- Adjusted to the split of plocation.number change
- Further style fixes
- Use platform dependant SIZE_T_PRINTF instead of hardcoded "%lu"

#18 Updated by Marko Lindqvist 2 months ago

Applied current patch (locally) to freeciv-web, and it breaks (first noticed from the fact that units are not moving to the direction requested). freeciv-web uses json + no-delta.

#19 Updated by Marko Lindqvist 2 months ago

Further split json protocol dio_put_*() API (return value) change -> https://osdn.net/projects/freeciv/ticket/44801

#20 Updated by Marko Lindqvist 2 months ago

Marko Lindqvist wrote:

Further split json protocol dio_put_*() API (return value) change -> https://osdn.net/projects/freeciv/ticket/44801

Attached is what is left of this patch after that split. With those 44801 parts not stealing attention, there's some obvious bugs in the remaining code; mostly that many functions just initialize e = -1 (i.e. error) and never touch it before returning it.

#22 Updated by Alina L. about 1 month ago

Split out the part replacing free(...); ... = NULL; with FC_FREE(...); -> https://osdn.net/projects/freeciv/ticket/44993

#23 Updated by Marko Lindqvist about 1 month ago

Here's the final patch, with what's left after all the other tickets, rebased and with some adjusment.

#24 Updated by Marko Lindqvist 25 days ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF