First rough draft of adding support for promises. #181

jimlloyd · 2015-01-06T23:23:26Z

@joeferner: Hi Joe, I managed to get something working for Issue #180.

This only promisifies the instance methods of the imported java classes, but adding support for static methods and promisifying the Java module itself is probably trivial in comparison.

Please review the changes in java.cpp and javaObject.cpp carefully. This implementation was almost blind trial and error without really trying to understand v8 and NAN. In particular I haven't tried to study how to properly handle garbage collection so I may have made mistakes there.

TODO:

More tests.
Promisify static methods of Java classes.
Promisify the various async methods of the Java module (newInstance, callMethod, callStaticMethod, etc.)

jimlloyd · 2015-01-07T00:56:41Z

Whoops, the Travis build failed for all node v0.8 and v0.11, but passes for all node 0.10. I only tested on my dev MacBook with node 0.10. I just tested with v0.11 and reproduced the error. Hopefully there is some NAN trick we can apply to the code to make it work on all three node versions. I'll start looking into that, but please let me know if there is an obvious fix.

jimlloyd · 2015-01-07T03:43:46Z

The last two commit seemingly fix the build problems for all 9 cases in our test matrix, but actually there is still an intermittent failure on node 0.10 (at, least, it's easy to reproduce on that version. I have not seen it in several attempts on node 0.11):

promises-test
✔ Promises - create an instance of a class and call methods (getClassPromise & getNamePromise)
*** ERROR: Lost reference to the dynamic proxy. You must maintain a reference in javascript land using ref() and unref(). (0x100e23140) ***

@joeferner as you review the code, please look for causes of this ref/unref problem. I'll look deeper into it tomorrow.

…efinements.

jimlloyd · 2015-01-07T18:23:04Z

@joeferner I haven't made any progress on the ref/unref problem. It seems to be specific to node 0.10, as I have never seen it in many attempts with node 0.8 and 0.11, but it seems to happen at least half of the time with 0.10. Note I am testing with 0.8.28, 0.10.35, and 0.11.14.

I'm going to set this aside and work on some other tasks until you've had a chance to take a look and provide some feedback. Thanks.

joeferner · 2015-01-08T17:56:44Z

I think that "ERROR" is unrelated. If you look at master and run the tests. You'll get the same error. It will appear in random locations based on what the garbage collector is doing at the time.

joeferner · 2015-01-08T18:06:31Z

src/javaObject.cpp

We may want to fail hard here. I'm pretty sure everything after this will not like it if result is not a function. In checkJavaException node-java assert(false) after printing the error. It would be nice to throw an exception but I'm not sure if that's allowed in the New method.

Yes, this check was partly just my debug diagnostics. But if a user passes in an invalid promisify function, one that doesn't return a function, then we should catch the error here. I agree that assert(false) in acceptable in this case.

joeferner · 2015-01-08T18:08:56Z

It's looking really good. Is there anything else left? Looks like you handle the two big cases, instance methods and static methods.

jimlloyd · 2015-01-08T21:13:29Z

Ok, I see now that the error could be unrelated. It's a little disturbing that it happens so frequently during the promises test with node 0.10, but since it happens elsewhere it probably is safe to ignore.

jimlloyd · 2015-01-08T21:14:59Z

I still haven't promisified the async methods of Java module: newInstance, callMethod, callStaticMethod. I plan to implement those soon so we can include them in the first release of this feature.

…tead of 'suffix'.

Joe, I implemented an onJvmCreated callback so I could do the promisification from javascript instead of C++. Let me know if you have reservations about this approach.

jimlloyd · 2015-01-08T23:40:21Z

@joeferner If you are ok with the above commits then all that remains is to update the README.md.

Note, I changed asyncOptions.suffix -> asyncOptions.promiseSuffix as I anticipate we may want to allow changing the Sync suffix, and maybe add non-empty suffix for Async. Actually I have even more ambitious plans that I will propose to you later.

Also, see the commit comment for the last commit. I added an onJvmCreated callback to javascript. Any concerns about that approach?

jimlloyd · 2015-01-09T07:46:59Z

@joeferner I've updated the README.md. But in the process I tested with Q, which is probably the most commonly used promises library, and was surprised to discover that it doesn't work. It's a bit unsettling. Given that 2 of 3 libraries tested work, it may be reasonable to release as is. The incompatibility is documented in the README.md. But it might be worth holding the release until we understand the problem better, or have tested with more promises libraries. FYI, the error is:

promises-test
Assertion failed: (handle->InternalFieldCount() > 0), function Unwrap, file /Users/jim/.node-gyp/0.11.14/src/node_object_wrap.h, line 50.
Abort trap: 6 (core dumped)

The stack crawl looks like this:

Application Specific Information:
Assertion failed: (handle->InternalFieldCount() > 0), function Unwrap, file /Users/jim/.node-gyp/0.11.14/src/node_object_wrap.h, line 50.


Thread 0 Crashed:: Java: AWT-AppKit  Dispatch queue: com.apple.main-thread
0   libsystem_kernel.dylib          0x00007fff8fe84282 __pthread_kill + 10
1   libsystem_c.dylib               0x00007fff8e9d2b73 abort + 129
2   libsystem_c.dylib               0x00007fff8e99ac59 __assert_rtn + 321
3   nodejavabridge_bindings.node    0x00000001020d745f JavaObject::methodCall(v8::FunctionCallbackInfo<v8::Value> const&) + 2367
4   node                            0x000000010013a652 v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) + 162
5   node                            0x000000010015a4b0 v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) + 544
6   ???                             0x0000297877b060a2 0 + 45597380862114
7   ???                             0x0000297877b24b25 0 + 45597380987685
8   ???                             0x0000297877c9f967 0 + 45597382539623
9   ???                             0x0000297877b24b29 0 + 45597380987689
10  ???                             0x0000297877c9f7e1 0 + 45597382539233
11  ???                             0x0000297877c9f624 0 + 45597382538788
12  ???                             0x0000297877c9f40e 0 + 45597382538254
13  ???                             0x0000297877c14588 0 + 45597381969288
14  ???                             0x0000297877b1ef60 0 + 45597380964192
15  ???                             0x0000297877b1dd50 0 + 45597380959568
16  node                            0x00000001001b5ca1 v8::internal::Invoke(bool, v8::internal::Handle<v8::internal::JSFunction>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*) + 417
17  node                            0x0000000100123dcd v8::Function::Call(v8::Handle<v8::Value>, int, v8::Handle<v8::Value>*) + 221
18  node                            0x00000001004b8a66 node::MakeCallback(node::Environment*, v8::Handle<v8::Value>, v8::Handle<v8::Function>, int, v8::Handle<v8::Value>*) + 375
19  node                            0x00000001004b8dfc node::MakeCallback(v8::Isolate*, v8::Handle<v8::Object>, v8::Handle<v8::Function>, int, v8::Handle<v8::Value>*) + 103
20  nodejavabridge_bindings.node    0x00000001020da9dc NanCallback::Call(int, v8::Handle<v8::Value>*) const + 124 (nan.h:1715)
21  nodejavabridge_bindings.node    0x00000001020d9e19 MethodCallBaton::WorkComplete() + 233 (methodCallBaton.cpp:90)
22  nodejavabridge_bindings.node    0x00000001020dab12 NanAsyncExecuteComplete(uv_work_s*) + 18 (nan.h:1839)
23  node                            0x000000010051a02a uv__work_done + 175
24  node                            0x000000010051b7af uv__async_event + 62
25  node                            0x000000010051b922 uv__async_io + 136
26  node                            0x000000010052937d uv__io_poll + 1491
27  node                            0x000000010051bd92 uv_run + 267
28  node                            0x00000001004be8b8 node::Start(int, char**) + 336
29  node                            0x00000001000011f4 start + 52

jimlloyd · 2015-01-09T07:48:12Z

I hit 'Close and comment' by mistake, I meant to just submit the comment.

…rguments.

jimlloyd · 2015-01-14T19:07:24Z

This PR is superseded by PR #187.

First rough draft of adding support for promises.

6a6aa25

jimlloyd mentioned this pull request Jan 6, 2015

Support for Promises/A+? #180

Closed

jimlloyd added 2 commits January 6, 2015 17:29

Fix build on node 0.11.

35556d8

Use when instead of bluebird for compatiblity with node 0.8.

02a66a7

jimlloyd added 2 commits January 7, 2015 08:03

Promisify java class static methods on import.

2d50f13

SetAccessor to prohibit overwriting of asyncOptions, and some other r…

0c24d61

…efinements.

joeferner reviewed Jan 8, 2015
View reviewed changes

jimlloyd added 2 commits January 8, 2015 14:29

Assert if promisfy doesn't return a function. Use 'promiseSuffix' ins…

ec21cc8

…tead of 'suffix'.

Promisify three async methods of Java module.

8ed3b12

Joe, I implemented an onJvmCreated callback so I could do the promisification from javascript instead of C++. Let me know if you have reservations about this approach.

Document promises in the README.md.

812c665

jimlloyd closed this Jan 9, 2015

jimlloyd reopened this Jan 9, 2015

jimlloyd added 4 commits January 13, 2015 11:11

Document in testHelpers.js the tested promises implementations.

79a6b2f

Add promises test with many chained .then() calls.

6b4e403

Add promises test using when's ability to pass promises to function a…

eb27f78

…rguments.

Try alternate test in attempt to pass with java 6.

15084ca

jimlloyd mentioned this pull request Jan 14, 2015

Support for Promises/A+ #187

Merged

jimlloyd closed this Jan 14, 2015

jimlloyd deleted the feature/promises branch January 14, 2015 21:08

First rough draft of adding support for promises. #181

First rough draft of adding support for promises. #181

Uh oh!

Conversation

jimlloyd commented Jan 6, 2015

Uh oh!

jimlloyd commented Jan 7, 2015

Uh oh!

jimlloyd commented Jan 7, 2015

Uh oh!

jimlloyd commented Jan 7, 2015

Uh oh!

joeferner commented Jan 8, 2015

Uh oh!

joeferner Jan 8, 2015

Choose a reason for hiding this comment

Uh oh!

jimlloyd Jan 8, 2015

Choose a reason for hiding this comment

Uh oh!

joeferner commented Jan 8, 2015

Uh oh!

jimlloyd commented Jan 8, 2015

Uh oh!

jimlloyd commented Jan 8, 2015

Uh oh!

jimlloyd commented Jan 8, 2015

Uh oh!

jimlloyd commented Jan 9, 2015

Uh oh!

jimlloyd commented Jan 9, 2015

Uh oh!

jimlloyd commented Jan 14, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants