Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Fix Ruby client failure post Graal RC14 upgrade #725

Closed
AmitRoushan opened this issue Apr 23, 2019 · 16 comments
Closed

[Bug] Fix Ruby client failure post Graal RC14 upgrade #725

AmitRoushan opened this issue Apr 23, 2019 · 16 comments
Assignees
Labels
BeforeBarcelonaKubeCon Must be resolved before release (4/30) bug importance/high Degree of importance, not urgency urgency/medium Degree of urgency, not importance
Milestone

Comments

@AmitRoushan
Copy link
Collaborator

AmitRoushan commented Apr 23, 2019

Post Graal RC14 upgrade Ruby client with Ruby micro service support need fix.
Following is issue faced:

  /home/roo1/development/DCAP-Sapphire/examples/hanksTodoRuby/src/main/ruby/amino/run/appexamples/hankstodo/hanks_todo_main.rb:22:in `main': The value 'DynamicObject@115c946b<Array>' cannot be passed from one context to another. The current context is 0x79ca7bea and the argument value originates from context 0x54f6b629. (ArgumentError)
     from /home/roo1/development/DCAP-Sapphire/examples/hanksTodoRuby/src/main/ruby/amino/run/appexamples/hankstodo/hanks_todo_main.rb:40:in `<main>'

Umbrella issue #658

@AmitRoushan AmitRoushan self-assigned this Apr 23, 2019
@AmitRoushan AmitRoushan added the BeforeBarcelonaKubeCon Must be resolved before release (4/30) label Apr 23, 2019
@AmitRoushan AmitRoushan added this to the RC7 milestone Apr 23, 2019
@AmitRoushan AmitRoushan added bug importance/high Degree of importance, not urgency urgency/medium Degree of urgency, not importance labels Apr 23, 2019
@AmitRoushan
Copy link
Collaborator Author

AmitRoushan commented Apr 25, 2019

FYI : primitive data types are working fine. Issues are there in composite or user defined data type
Similar issue : oracle/graal#631

@AmitRoushan
Copy link
Collaborator Author

AmitRoushan commented Apr 26, 2019

As mentioned above ruby client - ruby server scenario works well with primitive type.
Root cause for Composite/UserDefine type failure:
App stub(TodoListManager_Stub) in multi language uses a org.graalvm.polyglot.Context instance for deserialization. It is able to successfully deserialize it but when it try to return it to Ruby Client, Ruby Client tries to parse it with different context. Till GraalVM RC8 the support was available to pass value from one context to other but after RC9 only primitive, host and proxy objects are allowed.

To validate it, We have tested HanksTodoRuby app with RC6. RC7 and RC8. It works fine till RC8. We also tested HanksTodoRuby app with RC16 and it fails with user defined class.

Change log in RC9: https://github.com/oracle/graal/blob/master/sdk/CHANGELOG.md#version-10-rc9

@prostil
Copy link
Contributor

prostil commented Apr 29, 2019

This issue can be closed as it is being tracked by another issue #658

@prostil prostil closed this as completed Apr 29, 2019
@quinton-hoole
Copy link
Collaborator

Re-opening, as we can't remain stuck on RC8 forever. RC19 is now out. We need to fix this. Hopefully we can use a single Context, and the cross-context problem will go away.

@quinton-hoole quinton-hoole reopened this May 30, 2019
@quinton-hoole quinton-hoole self-assigned this May 30, 2019
@maheshrajus
Copy link
Collaborator

maheshrajus commented Jun 6, 2019

@quinton-hoole :

  1. I tested with GraalVM RC19 version and issue(Illigeal argument Exception) still exist with composite type arguments.

  2. Tested KvStoreJS example with RC19 GraalVm.

    image

    image

  3. As per your comment related to single conext ([Bug] Fix Ruby client failure post Graal RC14 upgrade #725 (comment)), I am checking workaround. Please give me any suggestions on the same.
    Thank you!

@quinton-hoole
Copy link
Collaborator

I have not looked into it at all. I just understood that we have two Graal Contexts and are trying to access between them, which is not allowed. Is it possible to place the objects in the same Graal Context?

@maheshrajus
Copy link
Collaborator

@quinton-hoole : I am not sure about placing objects in the single Graal Context. I am checking it currently. I will confirm it after my analysis.

@maheshrajus
Copy link
Collaborator

@quinton-hoole : As per discussion with the graalvm community we can not serialize and use the same context at the client side. They have given some other way to achieve this. We have to convert it to JSON string and send to client side and use it. I am doing this workaround currently.

Discussion on Gitter: https://gitter.im/graalvm/graal-core :
Christian Humer @chumer 20:06
@maheshrajus No we don’t currently support serialization of polyglot contexts or values.
You may convert objects to a string representation like json to send objects back and forth between server/client.
Mahesh Raju Somalaraju @maheshrajus Jun 07 20:12
ok, thank you @chumer

@quinton-hoole
Copy link
Collaborator

@maheshrajus Please avoid using JSON as the performance is (highly likely to be) very poor. Please use swift or protobuf instead (or at the very least measure the performance difference between these three before proceeding).

@quinton-hoole
Copy link
Collaborator

Also, why is this only a problem for one particular language?

@maheshrajus
Copy link
Collaborator

@maheshrajus Please avoid using JSON as the performance is (highly likely to be) very poor. Please use swift or protobuf instead (or at the very least measure the performance difference between these three before proceeding).

@quinton-hoole : While converting to JSON string,I received some exceptions and checked the same with Graal community people. context object is not data object we can not convert to json. We have to check some other workaround other than single context.

image
image
image

@maheshrajus
Copy link
Collaborator

Also, why is this only a problem for one particular language?

Yeah, I checked with java client with JS server and it is working perfectly. In this case also we are passing the non-primitive data object to context obj which created at the client side for deserializing the data. I am continuing my further analysis.

image
image

quinton-hoole added a commit to quinton-hoole/Amino.Run that referenced this issue Sep 13, 2019
1. Upgrade required GraalVM version
2. Fix GraalStubGenerator to work correctly with new GraalVM version.
3. Fix Graal unit tests to be compliant with ECMA script specification, as required by new GraalVM version.
quinton-hoole added a commit to quinton-hoole/Amino.Run that referenced this issue Sep 13, 2019
1. Upgrade required GraalVM version
2. Fix GraalStubGenerator to work correctly with new GraalVM version.
3. Fix Graal unit tests to be compliant with ECMA script specification, as required by new GraalVM version.
@quinton-hoole
Copy link
Collaborator

To clarify, #817 does not fix this issue.

The automated example scripts were simply not running the problematic case, and hence no errors were reported.

The reported error is still as before, when run correctly:

$ ./gradlew :examples:hanksTodoRuby:runjsapp

> Task :examples:hanksTodoRuby:runjsapp
Warning: '--jvm.*' options are deprecated, use '--vm.*' instead.
constructor
constructor
OK_add_todo
OK_add_todo
OK_add_todo
java.lang.IllegalArgumentException: The value 'DynamicObject<JSArray>@2796aeae' cannot be passed from one context to another. The current context is 0xb4711e2 and the argument value originates from context 0x1fa1cab1.
        at com.oracle.truffle.polyglot.PolyglotLanguageContext.migrateValue(PolyglotLanguageContext.java:861)
        at com.oracle.truffle.polyglot.PolyglotLanguageContext.migrateHostWrapper(PolyglotLanguageContext.java:872)

I will now get to work on fixing the above, as well as making sure that the automated tests run all the required scenarios (i.e. all client languages, against all server languages, and with non-primitive types).

@quinton-hoole
Copy link
Collaborator

@maheshrajus Did you get any further with your analysis of this issue?

@quinton-hoole
Copy link
Collaborator

@maheshrajus By the way, great work on this so far!

@quinton-hoole
Copy link
Collaborator

@maheshrajus Unfortunately it seems that there was a bug in the test you used to verify that java client to js server was working correctly. It seems that two unrelated instances of Date are being created and initialized to the current time. It just happened that the current time was within the same second, and the resolution of the printed output therefore made them look identical. I fixed the test as per below, and the values of the Date are indeed different:
Code:

            if (i % 2 == 0) val = new Date();
            else val = new TestClass(i);

            String valStr = val instanceof Date ? String.valueOf(((Date)val).getTime()) : String.valueOf(val);
            System.out.println(String.format("<client>: setting %s = %s", key, valStr));
            store.set(key, val);
            val = store.get(key);
            valStr = val instanceof Date ? String.valueOf(((Date)val).getTime()) : String.valueOf(val);
            System.out.println(String.format("<client>: got value %s with key %s", valStr, key));

Output:

<client>: setting key_2 = 1568825290723
Sep 18, 2019 9:48:10 AM amino.run.policy.scalability.LoadBalancedMasterSlaveBase$ClientPolicy onRPC
INFO: Sending request to master server localhost/127.0.0.1:22345
Sep 18, 2019 9:48:10 AM amino.run.policy.scalability.LoadBalancedMasterSlaveBase$ClientPolicy onRPC
INFO: Sending request to master server localhost/127.0.0.1:22345
<client>: got value 1568825290818 with key key_2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BeforeBarcelonaKubeCon Must be resolved before release (4/30) bug importance/high Degree of importance, not urgency urgency/medium Degree of urgency, not importance
Projects
None yet
Development

No branches or pull requests

4 participants