Add DcJsonrpcInstance class #2479

link2xt · 2023-02-28T10:10:15Z

TODO:

Parse responses.
Dispatch responses via SettableFuture.

The purpose of this is to make it possible to use JSON-RPC calls when the corresponding CFFI is not ready yet, or skip adding CFFI for new calls completely.

This change enables jsonrpc core feature in ndk-make.sh.

Since core PR deltachat/deltachat-core-rust#4341 is merged, we don't have to switch event processing to use JSON-RPC when we create a JSON-RPC instance. So we can keep event loop using the C API and use JSON-RPC for something simple at first.

For example, we can try to use it for QR-code scanning first to get proper errors on failure. Another option is to use it for event reception, so we receive proper JSON events instead of data1, data2 etc. Another option is to use it instead of the recently added in #2539 dc_http_response_t API.

Java API for JSON-RPC instance is ready, but we need a way to dispatch . ~
java.util.concurrent.CompletableFuture(UPDATE: not compatible with Android API 16) looks like something similar to the asyncio.Future we use in the Python JSON-RPC client.

r10s · 2023-02-28T16:06:53Z

why is that needed currently? that would also require core to be compiled with jsonrpc, right? (i see it is a draft, but still :)

link2xt · 2023-02-28T16:16:50Z

This PR enables jsonrpc feature in ndk-make.sh already.

It is not needed currently, but there are some ideas to start using JSON-RPC on all platforms. One suggestion is to try using it for QR-code scanning first to get proper errors on failure. Another option is to use it for event reception, so we receive proper JSON events instead of data1, data2 etc.

github-actions · 2023-02-28T17:41:36Z

To test the changes in this pull request, install this apk:
📦 app-preview.apk

r10s · 2023-03-01T15:07:20Z

tbh, i am unsure about getting jsonrpc in that quickly - i have the feeling it is not yet settled and stable - i see quite some breaking changes and and lots of pr in core, maybe it is wise to postpose that a little longer.

for qr code scanning, i think it is easily doable to have a "classic" ffi (in fact, we already did that at deltachat/deltachat-core-rust#3489 )

still interesting to test in a pr, however, to see how much apk grows (if it is only a few kb, the decision may be easier to have both for some time)

also, i'd like to run the benchmark script of @Hocuri at some point and see how much performance changes by using jsonrpc , eg. for the chatlist (that would need some more refactoring, of course)

but again, i do not sth of that as urgent things - but why is that combined with Nix changes then?

link2xt · 2023-03-01T15:19:07Z

but why is that combined with Nix changes then?

Nix is not related to JSON-RPC, I just use it for my development environment. Previously I used podman but it is not really suitable for development, only for building.

r10s · 2023-03-01T15:22:02Z

a yes, i noticed too late that #2483 is actually on master and just shown wrongly as "closed" on github.

so, yes, makes sense to try jsonrpc to get some findings and impressions, and once really needed we can merge that to master. i am still sceptical to make changes to the event system on master these days, without real need, however :)

hpk42 · 2023-03-02T10:54:49Z

IMO it makes sense to start being able to use jsonrpc also with Android (and iOS). it's certainly going to take a longer while until a serious attempt at porting existing CFFI calls to jsonrpc can/should take place. But if there is at least a base mechanism it becomes possible to play with it much better, also from DeltaLab etc.

link2xt · 2023-03-02T12:14:48Z

Currently JSON-RPC instance takes over the event channel, it consumes all events and transforms them into JSON-RPC notifications. We could modify the core to have an option to disable it, but I think it is more interesting to start with using JSON-RPC for events, so we can use rich event structures with meaningful variable names rather than text1 and text2.

EDIT 2023-04-22: JSON-RPC instance consuming events is going to be removed in deltachat/deltachat-core-rust#4341

This reverts commit 91ba9ae.

github-actions · 2023-04-22T19:09:29Z

To test the changes in this pull request, install this apk:
📦 app-preview.apk

Printing JSON-RPC responses for now.

github-actions · 2023-04-22T20:34:11Z

To test the changes in this pull request, install this apk:
📦 app-preview.apk

link2xt · 2023-04-22T21:00:41Z

Current state: spams logcat with

04-22 23:00:08.775 14723 22178 I ApplicationContext: Sending request: {"jsonrpc":"2.0","method":"sleep","params":[5.0],"id":1}
04-22 23:00:08.776 14723 22178 I ApplicationContext: Got JSON-RPC response: {"jsonrpc":"2.0","id":null,"error":{"code":-32700,"message":"expected value at line 1 column 1"}}
04-22 23:00:08.776 14723 22178 I ApplicationContext: Sending request: {"jsonrpc":"2.0","method":"sleep","params":[5.0],"id":1}
04-22 23:00:08.776 14723 22178 I ApplicationContext: Got JSON-RPC response: {"jsonrpc":"2.0","id":null,"error":{"code":-32700,"message":"data did not match any variant of untagged enum Message"}}
04-22 23:00:08.777 14723 22178 I ApplicationContext: Sending request: {"jsonrpc":"2.0","method":"sleep","params":[5.0],"id":1}
04-22 23:00:08.777 14723 22178 I ApplicationContext: Got JSON-RPC response: {"jsonrpc":"2.0","id":null,"error":{"code":-32700,"message":"expected value at line 1 column 1"}}
04-22 23:00:08.778 14723 22178 I ApplicationContext: Sending request: {"jsonrpc":"2.0","method":"sleep","params":[5.0],"id":1}
04-22 23:00:08.778 14723 22178 I ApplicationContext: Got JSON-RPC response: {"jsonrpc":"2.0","id":null,"error":{"code":-32700,"message":"data did not match any variant of untagged enum Message"}}
04-22 23:00:08.779 14723 22178 I ApplicationContext: Sending request: {"jsonrpc":"2.0","method":"sleep","params":[5.0],"id":1}
04-22 23:00:08.779 14723 22178 I ApplicationContext: Got JSON-RPC response: {"jsonrpc":"2.0","id":null,"error":{"code":-32700,"message":"expected value at line 1 column 1"}}
04-22 23:00:08.779 14723 22178 I ApplicationContext: Sending request: {"jsonrpc":"2.0","method":"sleep","params":[5.0],"id":1}

github-actions · 2023-04-22T21:20:50Z

To test the changes in this pull request, install this apk:
📦 app-preview.apk

link2xt

LGTM, but APIs are currently unused, would be nice to use at least one of them in this PR as an example, e.g. addAccount

src/com/b44t/messenger/rpc/Rpc.java

link2xt · 2023-04-27T11:36:07Z

src/com/b44t/messenger/rpc/Rpc.java

+        }
+    }
+
+    public int addAccount() throws RpcError {


Is this new API unused? Maybe use at least addAccount as an example of JSON-RPC usage?

yes it is unused, I just add it as example so when adding new API the dev knows how to add them, I could try using addAccount()

github-actions · 2023-04-27T11:55:12Z

To test the changes in this pull request, install this apk:
📦 app-preview.apk

Hocuri

I'm wondering if we should do some measurements before merging this (how much bigger does the APK get? How much longer does a native call take now that we have to serialize and deserialize things? How much does stuff like dc_get_http_response get faster since we can return the result as JSON instead of needing multiple follow-up calls like dc_http_response_get_mimetype?).

src/com/b44t/messenger/rpc/Rpc.java

Hocuri · 2023-04-27T13:17:26Z

src/com/b44t/messenger/rpc/Rpc.java

+            id = ++requestId;
+        }
+        String jsonRequest = gson.toJson(new Request(method, params, id));
+        CompletableFuture<JsonElement> future = new CompletableFuture<>();


Android Studio complains that CompletableFuture requires API level 24 (current min is 16; also see apilevels.com).

Did you test this on Android 6 and below yet?

BTW, there is a SettableFuture implementation in DC, taken from the Signal code, maybe it can be used here.

yikes I was using SettableFuture at first, but changed to CompletableFuture because was nicer and in the standard lib, so that is why we had that SettableFuture, will change it back, thanks a lot!

Maybe document this in the SettableFuture then? It is indeed "Added in API level 24" on https://developer.android.com/reference/java/util/concurrent/CompletableFuture, this page can be linked from there.

Another question is, why doesn't our CI complain or fail? If Android Studio knows this, it should be possible to detect this automatically from gradle build.

src/com/b44t/messenger/rpc/Rpc.java

Hocuri · 2023-04-27T15:14:40Z

src/com/b44t/messenger/rpc/Rpc.java

+        String jsonResponse = dcJsonrpcInstance.getNextResponse();
+        Response response = gson.fromJson(jsonResponse, Response.class);
+
+        if (response.id == 0) { // Got JSON-RPC notification, ignore


For my understanding, what's a JSON-RPC notification?

some legacy thing I think, in the past the RPC used to send unsolicited notifications with events so "notifications" here means incoming events that come without the client requesting them to the RPC server, we now have a new api to manually poll for events

in theory that check should never be true, I guess it was more about catching bugs during the creation of this PR by @link2xt and probably can be removed, maybe there is still a way to enable that notifications/events and that is why this check exist but in practice it should never be enabled

Would be nice to either replace notification with event or remove this block.

https://www.jsonrpc.org/specification#notification says "A Notification is a Request object without an "id" member."

There is no such thing as "event" in the specification. We previously used notifications to send events from the server to the client, but JSON-RPC 2.0 specification is written in a way assuming that only the client sends requests (including notifications, i.e. requests without an ID).

src/com/b44t/messenger/rpc/RpcError.java

Hocuri · 2023-04-27T15:28:14Z

build.gradle

    implementation 'com.fasterxml.jackson.core:jackson-databind:2.11.1' // used as JSON library
+    implementation 'com.google.code.gson:gson:2.10.1' // used as JSON library


Hm, so we need one more JSON library? jackson-databind doesn't work? Can we use gson instead of jackson-databind, in order to not increase the number of dependencies?

if you grep for gson, you will find that we already use it in some part of the code, it seems it is a dependency of some of our dependencies already, I had to explicitly add this because it seems the version used by the dependency was old and didn't have a needed method.

in a follow-up PR we should try to only use one of gson, jackson and org.json (the 3 are used at the moment in existing code besides this PR)

Hocuri · 2023-04-27T15:38:03Z

src/com/b44t/messenger/rpc/Rpc.java

+        if (response.error != null) {
+            future.completeExceptionally(new RpcError(response.error.toString()));
+        } else if (response.result != null) {
+            future.complete(response.result);
+        } else {
+            future.completeExceptionally(new RpcError("Got JSON-RPC response witout result or error: " + jsonResponse));
+        }


There's a lot of code changing exceptions from one form to another here, what if we replaced this whole block with future.complete(response);, used CompletableFuture<Response> instead of CompletableFuture<JsonElement>, and let the callers (like getResult()) extract the error on their own.

Also, creating a Throwable is always expensive, since it captures a stack trace. Idk how expensive excactly and if it might be noticeable in our code.

all the callers will want to extract the error/result so it would be too much of a burden and repeated code, they have to do this anyway, and if there is an error they will want to print it in the logs, because it will be a programming error (ex. bad usage of API)

(these are not incoming events that could become a bunch of errors, they are only responses to API calls, so if you call a function and it throws an exception you need to handle it anyway)

But checking for response.result != null is the same amount of code as catching the error

src/com/b44t/messenger/rpc/Rpc.java

Co-authored-by: Hocuri <hocuri@gmx.de>

github-actions · 2023-04-27T17:44:39Z

To test the changes in this pull request, install this apk:
📦 app-preview.apk

github-actions · 2023-04-27T19:07:55Z

To test the changes in this pull request, install this apk:
📦 app-preview.apk

r10s · 2023-04-27T21:28:17Z

I'm wondering if we should do some measurements before merging this (how much bigger does the APK get? How much longer does a native call take now that we have to serialize and deserialize things? How much does stuff like dc_get_http_response get faster since we can return the result as JSON instead of needing multiple follow-up calls like dc_http_response_get_mimetype?).

yeah, that'd be interesting, however, in general, for non-time-critical things, jsonrpc probably makes sense anyway.

dc_get_http_response would be a perfect example to test that out in this pr, btw - it is not time critical, not overall too critical, has quite some overhead with ffi

for adding accounts, not sure, i would not touch that for now.

also, other than stated at deltachat/deltachat-ios#1847 "the aim is to start by using JSON-RPC for any new API" - i am not sure if that is really the aim for now. eg. if there comes a new method to dc_msg_t, it probably makes sense to add that to ffi still. also, for reading eg. a single record, it has to be proven if that is faster or much easier i'd say.

i see this jsonprc more as something that may sneak in, if it turns out that things are easier and faster with that, this is the way automatically. iirc, jsonrpc could also get a bit more mature before being used everywhere, iirc there is no mapping between ffi and jsonrpc, low-level things and documentation are also missing. also from that perspective, there is no need to hurry.

adbenitez · 2023-05-01T18:25:26Z

dc_get_http_response would be a perfect example to test that out in this pr, btw - it is not time critical, not overall too critical, has quite some overhead with ffi

ok, done ☑️

github-actions · 2023-05-01T18:47:39Z

To test the changes in this pull request, install this apk:
📦 app-preview.apk

adbenitez · 2023-05-01T19:00:07Z

also, other than stated at deltachat/deltachat-ios#1847 "the aim is to start by using JSON-RPC for any new API" - i am not sure if that is really the aim for now. eg. if there comes a new method to dc_msg_t, it probably makes sense to add that to ffi still.

yip for something like a new method to dc_msg_t wouldn't make sense, mean it like for new classes etc.

ideally at some point there will be automatic generation of the bindings then we can make more heavy migration

r10s · 2023-06-07T19:17:10Z

ftr, as the question of size popped up above: with 1.37, we have some measurements:

android1.37 is 11.6 mb larger than android1.36 (+24%) :/
checked the reasons a bit:
plus 6.8 mb for jsonrpc in core (tested by leaving out "--features jsonrpc")
plus 4.7 mb other core growings - updated dependecies? idk (tested by compiling 1.37 against core112.8
plus <0.1 mb growing java code
i find that pretty surprisingly, compared to other features we added recently (iroh) -- maybe some dependencies are doubled by jsonrpc or so?

r10s · 2023-06-08T11:19:39Z

I'm wondering if we should do some measurements before merging this

ftr, there are some measurements at deltachat/deltachat-core-rust#4463 now

they are replaced by jsonrpc in #2479

link2xt force-pushed the link2xt/jsonrpc branch 3 times, most recently from f70c7f5 to 95ea5cd Compare February 28, 2023 15:47

link2xt force-pushed the link2xt/jsonrpc branch 2 times, most recently from 07018e0 to 85a84c0 Compare March 1, 2023 12:03

link2xt mentioned this pull request Mar 1, 2023

Add Nix development environment #2483

Closed

link2xt force-pushed the link2xt/jsonrpc branch from 85a84c0 to 6fcd461 Compare March 1, 2023 15:04

link2xt force-pushed the link2xt/jsonrpc branch 3 times, most recently from 13b3329 to cff98f6 Compare April 22, 2023 15:51

link2xt added 4 commits April 22, 2023 17:58

Add DcJsonrpcInstance class

4f468a4

Start using DcJsonrpcInstance

1405ae0

Revert "Start using DcJsonrpcInstance"

3980073

This reverts commit 91ba9ae.

Add dcJsonrpcInstance to ApplicationContext

00fd1a9

link2xt force-pushed the link2xt/jsonrpc branch from cff98f6 to 00fd1a9 Compare April 22, 2023 17:58

Remove getNextEventCPtr

5246ee7

Add JSON-RPC thread

39e9ab8

Printing JSON-RPC responses for now.

Try to send JSON-RPC requests

a7e6f48

link2xt commented Apr 27, 2023

View reviewed changes

link2xt requested a review from r10s April 27, 2023 11:37

link2xt marked this pull request as ready for review April 27, 2023 11:37

Hocuri reviewed Apr 27, 2023

View reviewed changes

adbenitez and others added 2 commits April 27, 2023 13:19

Apply suggestions from code review

e1d0712

Co-authored-by: Hocuri <hocuri@gmx.de>

check if future is null

e4beb06

adbenitez added 3 commits April 27, 2023 14:25

move SettableFuture to core package

7935513

tweak comment

a5b1528

rename RpcError to RpcException

cbec549

adbenitez requested a review from Hocuri April 27, 2023 18:51

adbenitez mentioned this pull request Apr 27, 2023

start using JSON-RPC on iOS deltachat/deltachat-ios#1847

Closed

add getHttpResponse to Rpc

0b85cd2

adbenitez merged commit 4461358 into master May 5, 2023

adbenitez deleted the link2xt/jsonrpc branch May 5, 2023 15:57

adbenitez restored the link2xt/jsonrpc branch May 5, 2023 15:58

adbenitez deleted the link2xt/jsonrpc branch May 5, 2023 15:58

r10s mentioned this pull request May 5, 2023

removed old dc_http_response_t and related functions #2553

Closed

r10s mentioned this pull request Jun 8, 2023

size increase of core116 deltachat/deltachat-core-rust#4463

Closed

r10s added a commit that referenced this pull request Sep 25, 2023

remove unused DcHttpResponse and related functions

f57a659

they are replaced by jsonrpc in #2479

r10s mentioned this pull request Sep 25, 2023

remove unused DcHttpResponse and related functions #2665

Merged

r10s added a commit that referenced this pull request Sep 25, 2023

remove unused DcHttpResponse and related functions

0f09aba

they are replaced by jsonrpc in #2479

		implementation 'com.fasterxml.jackson.core:jackson-databind:2.11.1' // used as JSON library
		implementation 'com.google.code.gson:gson:2.10.1' // used as JSON library

Add DcJsonrpcInstance class #2479

Add DcJsonrpcInstance class #2479

Conversation

link2xt commented Feb 28, 2023 • edited by adbenitez Loading

r10s commented Feb 28, 2023

link2xt commented Feb 28, 2023

github-actions bot commented Feb 28, 2023

r10s commented Mar 1, 2023 • edited Loading

link2xt commented Mar 1, 2023

r10s commented Mar 1, 2023 • edited Loading

hpk42 commented Mar 2, 2023 via email

link2xt commented Mar 2, 2023 • edited Loading

github-actions bot commented Apr 22, 2023

github-actions bot commented Apr 22, 2023

link2xt commented Apr 22, 2023

github-actions bot commented Apr 22, 2023

link2xt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 27, 2023

Hocuri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adbenitez Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 27, 2023

github-actions bot commented Apr 27, 2023

r10s commented Apr 27, 2023 • edited Loading

adbenitez commented May 1, 2023

github-actions bot commented May 1, 2023

adbenitez commented May 1, 2023 • edited Loading

r10s commented Jun 7, 2023

r10s commented Jun 8, 2023

link2xt commented Feb 28, 2023 •

edited by adbenitez

Loading

r10s commented Mar 1, 2023 •

edited

Loading

r10s commented Mar 1, 2023 •

edited

Loading

link2xt commented Mar 2, 2023 •

edited

Loading

adbenitez Apr 27, 2023 •

edited

Loading

r10s commented Apr 27, 2023 •

edited

Loading

adbenitez commented May 1, 2023 •

edited

Loading