Make connection to CodeGeneratorService lazy. #509

ChrisCanCompute · 2017-01-28T13:12:59Z

Fixes #37

This change is

AnmAtAnm · 2017-01-30T16:36:05Z

blocklylib-core/src/main/java/com/google/blockly/android/codegen/LazyServiceConnection.java

+
+import com.google.blockly.android.codegen.CodeGeneratorService.CodeGeneratorBinder;
+
+class LazyServiceConnection {


JavaDoc
Maybe rename class as LazyServiceWrapper (your call).

AnmAtAnm · 2017-01-30T16:43:14Z

blocklylib-core/src/main/java/com/google/blockly/android/codegen/LazyServiceConnection.java

+        if (isBound()) {
+            executeCodeGenerationRequest(codeGenerationRequest);
+        } else {
+            mStoredRequest = codeGenerationRequest;


Looks like there is a risk of overwriting a prior value of mStoredRequest. To avoid dropping multiple requests, mStoredRequests probably needs to be a queue. That queue may need to be copied to the Service object.

(That said, our callback does not have a mechanism for differentiating multiple requests is the same callback function is used.)

I think this is an edge case, but I've added a queue anyway.
If you use different callback objects with different code generation requests then this all works as expected.

AnmAtAnm · 2017-01-30T16:58:24Z

blocklylib-core/src/main/java/com/google/blockly/android/codegen/CodeGeneratorManager.java

+                        codeGenerationCallback,
+                        blockDefinitionsJsonPaths,
+                        generatorsJsPaths);
+                mCodeGenerationConnection.requestCodeGeneration(codeGenerationRequest);


Revisiting this code, the new CodeGenerationRequest() feels odd here, in that it doesn't seem to provide value to this class.

Option #1: It could be passed constructed previously, and then passed in. This eliminates the need for workspace in this class, and could be returned as part of the callback. I prefer this for that last point, but that would be a breaking change to the callback interface.

Option #2: The CodeGenerationRequest could be buried deeper, passing each of the parameters into the lazy service for construction there.

I went with 1, and discovered that it removes all the logic from CodeGenerationManager, so I've done that, then relocated the lazy loading into CodeGenerationManager and deleted the new LazyServiceConnection class.

…nnection. Fixes google#37

ChrisCanCompute · 2017-01-30T21:10:23Z

blocklylib-core/src/main/java/com/google/blockly/android/codegen/CodeGeneratorManager.java

+            public void onServiceConnected(ComponentName className, IBinder binder) {
+                mGeneratorService = ((CodeGeneratorService.CodeGeneratorBinder) binder).getService();
+                while (!mStoredRequests.isEmpty()) {
+                    executeCodeGenerationRequest(mStoredRequests.poll());


I've used poll() rather than remove() here, as the private execute method can handle nulls.

ChrisCanCompute · 2017-01-30T21:10:32Z

blocklylib-core/src/main/java/com/google/blockly/android/codegen/CodeGeneratorManager.java

+            public void onServiceConnected(ComponentName className, IBinder binder) {
+                mGeneratorService = ((CodeGeneratorService.CodeGeneratorBinder) binder).getService();
+                while (!mStoredRequests.isEmpty()) {
+                    executeCodeGenerationRequest(mStoredRequests.poll());


I've used poll() rather than remove() here, as executeCodeGenerationRequest can handle nulls.

AnmAtAnm

If you're aiming to resolve #37, it is the bindService() that should be lazy. Sorry if the issue wasn't clear, and that I didn't catch this in the previous review.

AnmAtAnm · 2017-02-01T02:42:01Z

blocklylib-core/src/main/java/com/google/blockly/android/codegen/CodeGeneratorManager.java

    public void onResume() {
        Intent intent = new Intent(mContext, CodeGeneratorService.class);
-        mContext.bindService(intent, mCodeGenerationConnection, Context.BIND_AUTO_CREATE);
+        if (mCodeGenerationConnection != null) {
+            mContext.bindService(intent, mCodeGenerationConnection, Context.BIND_AUTO_CREATE);


To be clear, it is this bind that could be lazy, and is the point of issue #37. We should wait until the first requestCodeGeneration() call, since it is entirely possible Blockly is used without code generation, and a full background WebView is fairly heavyweight. Scratch Blocks is an example of this sort of usage.

That makes sense. I've made an update so that the bind is lazy (rather than creation of the connection).

Fixes google#37

ChrisCanCompute · 2017-02-01T06:54:44Z

I think there's potentially a risk now that bind gets called multiple times if the binding hasn't finished before more code generation is requested.

I'm not sure how the context will handle multiple binds of the same object. It might be a non-issue.

Update: From reading the docs, I think bind service is synchronous as it returns a boolean which represents if the bind was successful or not. Therefore I don't think the above is a problem.

RoboErikG

Added some notes about how to protect against race conditions and leftover requests. In practice, the service binding should be happening immediately since it's a local service, but it's good to add these if you're making changes to the service binding.

RoboErikG · 2017-02-01T17:38:11Z

blocklylib-core/src/main/java/com/google/blockly/android/codegen/CodeGeneratorManager.java

    public void onPause() {
-        mContext.unbindService(mCodeGenerationConnection);
+        if (isBound()) {


In order to deal with race conditions, including the user leaving and re-entering while a bind request is in progress, you'll want to keep track of the desired state.

In onPause you should also set a mResumed flag to false (and set it to true in onResume).

In onServiceConnected check if (mResumed). If not, disconnected immediately.

In onResume you should clear out any pending requests since you won't reconnect until a new request comes in.

RoboErikG · 2017-02-01T17:45:19Z

blocklylib-core/src/main/java/com/google/blockly/android/codegen/CodeGeneratorManager.java

-                Log.i(TAG, "Generator not bound to service. Skipping run request.");
-            }
+    public void requestCodeGeneration(CodeGenerationRequest codeGenerationRequest) {
+        if (isBound()) {


Add if(!mResumed) { Log.w(TAG, "Code generation called while paused. Request ignored."); return;}

RoboErikG · 2017-02-01T17:47:23Z

blocklylib-core/src/main/java/com/google/blockly/android/codegen/CodeGeneratorManager.java

        }
    }

    private boolean isBound() {
        return mGeneratorService != null;
    }
+
+    private void connectToService() {
+        Intent intent = new Intent(mContext, CodeGeneratorService.class);


Add a mIsConnecting flag. Set it to true after bindService is called and false when the service connects. In this method check if (!isBound() && !mIsConnecting) before calling bindService.

hard to reach scenarios. Fixes google#37

Fixes google#37

ChrisCanCompute · 2017-02-01T21:18:40Z

I've updated to handle these cases.

Thank you both for your detailed feedback!

RoboErikG · 2017-02-01T21:37:25Z

Review status: 0 of 2 files reviewed at latest revision, 11 unresolved discussions.

blocklylib-core/src/main/java/com/google/blockly/android/codegen/CodeGeneratorManager.java, line 40 at r4 (raw file):

                    mContext.unbindService(mCodeGenerationConnection);
                } else {
                    mGeneratorService = ((CodeGeneratorService.CodeGeneratorBinder) binder).getService();

nit line length

blocklylib-core/src/main/java/com/google/blockly/android/codegen/CodeGeneratorManager.java, line 41 at r4 (raw file):

                } else {
                    mGeneratorService = ((CodeGeneratorService.CodeGeneratorBinder) binder).getService();
                    mIsConnecting = false;

move mIsConnection = false to the top of the method so it's always done.

blocklylib-core/src/main/java/com/google/blockly/android/codegen/CodeGeneratorManager.java, line 89 at r4 (raw file):

        } else {
            mStoredRequests.add(codeGenerationRequest);
            if (!mIsConnecting) {

move this check into connectToService so the caller doesn't have to worry about it.

Comments from Reviewable

AnmAtAnm · 2017-02-02T16:41:53Z

blocklylib-core/src/main/java/com/google/blockly/android/codegen/CodeGeneratorManager.java

-                }
-            } else {
-                Log.i(TAG, "Generator not bound to service. Skipping run request.");
+    public void requestCodeGeneration(CodeGenerationRequest codeGenerationRequest) {


@NotNull CodeGenerationRequest codeGenerationRequest.

Nulls are already handled in the code, but I'll add a null check here so that that's clearer.

AnmAtAnm · 2017-02-02T16:42:57Z

blocklylib-core/src/main/java/com/google/blockly/android/codegen/CodeGeneratorManager.java

-            } else {
-                Log.i(TAG, "Generator not bound to service. Skipping run request.");
+    public void requestCodeGeneration(CodeGenerationRequest codeGenerationRequest) {
+        if (isBound()) {


Check resumed state first.

AnmAtAnm · 2017-02-02T17:00:55Z

blocklylib-core/src/main/java/com/google/blockly/android/codegen/CodeGeneratorManager.java

+                    mContext.unbindService(mCodeGenerationConnection);
+                } else {
+                    mGeneratorService = ((CodeGeneratorService.CodeGeneratorBinder) binder).getService();
+                    mIsConnecting = false;


Move mIsConnecting = false; to the end, outside of the if (!mResumed) check. Maybe even in a finally block.

AnmAtAnm · 2017-02-02T17:11:44Z

blocklylib-core/src/main/java/com/google/blockly/android/AbstractBlocklyActivity.java

+            }
+        } else {
+            Log.i(TAG, "No blocks in workspace. Skipping run request.");
+        }


Because this is going to have merge conflicts with some upcoming changes, I've updated the PR to target the toolbox-refactor branch. I'd appreciate if I can have write access to this branch to fix the upcoming conflict.

Thanks.

I think I've done this, but let me know if there are still issues (the merge conflicts were all imports, so trivial to resolve).

…ServiceLazy

Fixes google#37

googlebot · 2017-02-04T05:04:26Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

Make connection to CodeGeneratorService lazy.

8d9f6a0

Fixes google#37

AnmAtAnm suggested changes Jan 30, 2017

View reviewed changes

AnmAtAnm assigned ChrisCanCompute Jan 30, 2017

Move Lazy Loading into CodeGenerationManager and remove LazyServiceCo…

416f552

…nnection. Fixes google#37

ChrisCanCompute commented Jan 30, 2017

View reviewed changes

AnmAtAnm suggested changes Feb 1, 2017

View reviewed changes

Make the binding lazy (as that's the expensive call.

c1e534d

Fixes google#37

RoboErikG reviewed Feb 1, 2017

View reviewed changes

ChrisCanCompute added 2 commits February 1, 2017 21:11

Add checks against race conditions and handle backgrounding the app in

296e5c8

hard to reach scenarios. Fixes google#37

Swap setting of flag and checking of boolean to prevent race condition.

c64980b

Fixes google#37

AnmAtAnm reviewed Feb 2, 2017

View reviewed changes

AnmAtAnm approved these changes Feb 2, 2017

View reviewed changes

AnmAtAnm changed the base branch from master to toolbox-refactor February 2, 2017 18:35

ChrisCanCompute and others added 3 commits February 3, 2017 06:50

Merge branch 'toolbox-refactor' into 37-makeConnectionToCodeGenerator…

c72b767

…ServiceLazy

Further update to boolean checks.

5a895d9

Fixes google#37

Merge branch 'toolbox-refactor' into PR509

63467a9

AnmAtAnm merged commit 018607f into google:toolbox-refactor Feb 4, 2017

ChrisCanCompute deleted the 37-makeConnectionToCodeGeneratorServiceLazy branch February 6, 2017 11:50

AnmAtAnm mentioned this pull request Mar 2, 2017

CodeGeneratorService is started before first code generation request #37

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make connection to CodeGeneratorService lazy. #509

Make connection to CodeGeneratorService lazy. #509

ChrisCanCompute commented Jan 28, 2017 •

edited by RoboErikG

AnmAtAnm Jan 30, 2017

AnmAtAnm Jan 30, 2017

ChrisCanCompute Jan 30, 2017

AnmAtAnm Jan 30, 2017

ChrisCanCompute Jan 30, 2017

ChrisCanCompute Jan 30, 2017

ChrisCanCompute Jan 30, 2017

AnmAtAnm left a comment

AnmAtAnm Feb 1, 2017

ChrisCanCompute Feb 1, 2017

ChrisCanCompute commented Feb 1, 2017 •

edited

RoboErikG left a comment

RoboErikG Feb 1, 2017

RoboErikG Feb 1, 2017

RoboErikG Feb 1, 2017

ChrisCanCompute commented Feb 1, 2017

RoboErikG commented Feb 1, 2017

AnmAtAnm Feb 2, 2017

ChrisCanCompute Feb 3, 2017

AnmAtAnm Feb 2, 2017

AnmAtAnm Feb 2, 2017

AnmAtAnm Feb 2, 2017 •

edited

ChrisCanCompute Feb 3, 2017

googlebot commented Feb 4, 2017


		import com.google.blockly.android.codegen.CodeGeneratorService.CodeGeneratorBinder;

		class LazyServiceConnection {

Make connection to CodeGeneratorService lazy. #509

Make connection to CodeGeneratorService lazy. #509

Conversation

ChrisCanCompute commented Jan 28, 2017 • edited by RoboErikG

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

AnmAtAnm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisCanCompute commented Feb 1, 2017 • edited

RoboErikG 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

ChrisCanCompute commented Feb 1, 2017

RoboErikG commented Feb 1, 2017

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

AnmAtAnm Feb 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Feb 4, 2017

ChrisCanCompute commented Jan 28, 2017 •

edited by RoboErikG

ChrisCanCompute commented Feb 1, 2017 •

edited

AnmAtAnm Feb 2, 2017 •

edited