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

Fixed https://github.com/couchbase/couchbase-lite-java-core/issues/1375 #14

Merged
merged 2 commits into from Jul 29, 2016

Conversation

@hideki
Copy link

commented Jul 28, 2016

Issue: Reduce intermittently returns wrong results for 1.3.0-45

As posted in couchbase/couchbase-lite-java-core#1375 (comment), given keys and values are correct, but result from reduce function is incorrect. According to Rhino documentation (https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Rhino/Scopes_and_Contexts), Scope is sharable from multiple threads, but others are not. CBL Java/Android used to compile given JavaScript for every requests. But it makes JavaScript map/reduce function makes really slow. So we switched to "compile once, and reuse it" style. It seems accessing Function instance from multiple threads is not safe. So, by this fix, synchronized execution block.

Issue: Reduce intermittently returns wrong results for 1.3.0-45

As posted in couchbase/couchbase-lite-java-core#1375 (comment), given keys and values are correct, but result from reduce function is incorrect. According to Rhino documentation (https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Rhino/Scopes_and_Contexts), Scope is sharable from multiple threads, but others are not. CBL Java/Android used to compile given JavaScript for every requests. But it makes JavaScript map/reduce function makes really slow. So we switched to "compile once, and reuse it" style. It seems accessing `Function` instance from multiple threads is not safe. So, by this fix, synchronized execution block.
return null;
} finally {
org.mozilla.javascript.Context.exit();
synchronized (lockFunction) {

This comment has been minimized.

Copy link
@pasin

pasin Jul 28, 2016

Contributor

Does naiveSum() and nativeCount() already thread safe? Or not, maybe just simply synchronize the method?

This comment has been minimized.

Copy link
@hideki

hideki Jul 28, 2016

Author

I don't see any threading issue with nativeSum() and nativeCount(). These methods don't access any instance variables.

Object jsDocument = org.mozilla.javascript.Context.javaToJS(revision.getProperties(), localScope);
Object jsParams = org.mozilla.javascript.Context.javaToJS(params, localScope);

synchronized (lockFunction) {

This comment has been minimized.

Copy link
@pasin

pasin Jul 28, 2016

Contributor

Can we simply synchronize the method instead of using block to synchronize the whole method? I think there is no need to do the fine grain block sync.

This comment has been minimized.

Copy link
@hideki

hideki Jul 28, 2016

Author

I WAS NOT sure if synchronized Function.call() is enough. According to my test results, synchronized Function.call() is enough. I will update PR.

@@ -48,28 +55,29 @@ public ViewMapBlockRhino(String src) {

@Override
public void map(Map<String, Object> document, Emitter emitter) {
synchronized (lockFunction) {

This comment has been minimized.

Copy link
@pasin

pasin Jul 28, 2016

Contributor

Same here, I think we could just sync at the method level.

@hideki

This comment has been minimized.

Copy link
Author

commented Jul 28, 2016

@pasin please review this PR again.

@pasin pasin merged commit 7374ac8 into master Jul 29, 2016
@pasin pasin deleted the feature/issue_1375 branch Jul 29, 2016
@pasin pasin removed the in progress label Jul 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.