#167410: _initCalled completed twice

[srivats22]

This PR Fixes: flutter/flutter#167410, where _initCalled was being performed twice on the web

Based on the discussion comments I have removed the calles to _initCalled in the google_sign_in_web package

Pre-Review Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter.
• I signed the CLA.
• The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• I linked to at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under¹.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under¹.
• I updated/added any relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or I have commented below to indicate which test exemption this PR falls under¹.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

Footnotes

1. Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩² ↩³

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

[gemini-code-assist]

Code Review

The pull request removes the _initCalled completer, which introduces a race condition. The init method should be made idempotent to handle being called multiple times. The CHANGELOG.md entry should be a complete sentence ending with a period.

[srivats22]

Hi,

any idea or pointers on how to fix this:

The following TestFailure was thrown running a test (but after
the test had completed):
Expected: throws <Instance of 'StateError'>
  Actual: <Closure: () => Future<Null> from: () => {
                            let t$goto = 0, t$completer =
async._makeAsyncAwaitCompleter(T.Null());
                            var t$36asyncBody =
async._wrapJsFunctionForAsync((t$errorCode, t$result) => {
                              if (t$errorCode === 1) return
async._asyncRethrow(t$result, t$completer);
                              while (true)
                                switch (t$goto) {
                                  case 0:
                                    // Function start
                                    t$goto = 2;
                                    return
async._asyncAwait(t$36$35plugin$35get().disconnect(C[8] ||
CT.C8), t$36asyncBody, t$completer);
                                  case 2:
                                    // returning from await.
                                    // implicit return
                                    return
async._asyncReturn(null, t$completer);
                                }
                            });
                            return
async._asyncStartSync(t$36asyncBody, t$completer);
                          }>
   Which: returned a Future that emitted <null>

When the exception was thrown, this was the stack:

its failing for the same reason in 3 of the tests and the repo check I know the issue which I will fix...

[stuartmorgan-g]

I'm not sure I understand the question. The expectations that check that calling a method without calling init throws a StateError are failing because you removed the code that throws the StateError if a method is called without calling init.

[srivats22]

Oh so the changes in the PR is incorrect? Or something else needs to change?

[stuartmorgan-g]

Tests that expect that the web implementation asserts init completion everywhere are no longer valid if the web implementation no longer asserts init completion. Intentional behavioral changes often require changing tests.

[srivats22]

Understood let me look at the code and see where the initCompleted is still be called in the test and remove those... Last I checked wasn't able to find any

[stuartmorgan-g]

From triage: Is this ready for review?

[srivats22]

Yes its ready for review looks like I might need to fix the conflicts which I will get to

[mdebbar]

Thanks for contributing this fix!

Some thoughts:

When the app calls init() twice (potentially with different params), it becomes a tricky situation. We already have the first init() in flight while the second init() is being executed.

I think we should let both init() calls continue to completion. Any calls to await initialized; should await the latest init() call.

init(InitParameters(clientId: 111));
init(InitParameters(clientId: 222));
await initialized;
// At this point, the plugin should be ready with clientId 222.

@stuartmorgan-g what do other platforms do in this case?

[srivats22]

I made the changes based on what was put in the issue... What I got from it was since the logic has been moved outside the package the code for init seems redundant... I might have miss understood let me take a look based on the comments and try and incorporate it

[stuartmorgan-g]

@stuartmorgan-g what do other platforms do in this case?

Nothing; in the new version of google_sign_in, clients explicitly call an initialize method (rather than it being an internal, implicit thing on most platforms, but semi-exposed on web, as was the case before), and the docs say they must do so exactly once. It's a programming error by the client to write the code you've shown there.

If we think clients need explicit Errors when doing the wrong thing, that should be added to the app-facing package, rather than handled in each implementation. This code is a legacy of the exposed-for-web nature of the previous init code.

(Also, clients should basically never call the platform interface methods directly. That's not how most federated plugins are designed.)

[stuartmorgan-g]

This sounds like there was a bug in the plugin that was fixed, which is not the case. This should say something like "Removes initialization checks, since initialization handling has moved to the app-facing package."

[stuartmorgan-g]

This method should just be removed.

[srivats22]

Will work on it and push the updated changes by today if possible

[srivats22]

Removing the function completely won't work cause the renderButton uses the initialized function as the future

[stuartmorgan-g]

That code is wrong now though; initialized is now equivalent to just _jsSdkLoadedFuture in this PR, but the code in renderButton that is being guarded requires _gisSdkClient to be non-null, and there's no order guarantee between those two things.

If we want to keep renderButton callable before initialization (I'm skeptical that that's actually important with the new API structure), then the internals need to be restructured to make that actually work. But that can be done with an internal future that doesn't have a getter wrapping it.

[srivats22]

Understood so how should I proceed??

[stuartmorgan-g]

@mdebbar Do you want renderButton to be callable before the plugin client has run initialize at the app-facing package level?

[stuartmorgan-g]

From triage: Ping @mdebbar on the question above.

[mdebbar]

I looked at iOS and Android implementations and I didn't see explicit errors being thrown if init() is called twice.

The documentation says: or calling this method more than once, will result in undefined behavior. I would say throwing an error qualifies as "undefined behavior" :)

Here's my suggestion:

1. Make initialized a private property _initialized to be used by methods in this file, and returned from init() to be used by users.
2. Throw if init() is called more than once.
3. _gisSdkClient should become late final GisSdkClient _gisSdkClient; so that it can only be set once, and it throws if accessed before initialization.

[mdebbar]

Sorry for the delay in my response. I gave this some more thought and looked at the documentation and other platforms. See my suggestion in the comment below.

[mdebbar]

I looked at iOS and Android implementations and I didn't see explicit errors being thrown if init() is called twice.

The documentation says: or calling this method more than once, will result in undefined behavior. I would say throwing an error qualifies as "undefined behavior" :)

Here's my suggestion:

1. Make initialized a private property _initialized to be used by methods in this file, and returned from init() to be used by users.
2. Throw if init() is called more than once.
3. _gisSdkClient should become late final GisSdkClient _gisSdkClient; so that it can only be set once, and it throws if accessed before initialization.

[srivats22]

Thank you, I will work on it with the directions provided and update the PR

[stuartmorgan-g]

From triage: What is the status of this PR? It looks like there are still CI failures that need to be addressed.

[srivats22]

From triage: What is the status of this PR? It looks like there are still CI failures that need to be addressed.

I tried fixing it based on the failures but it kept comming back not sure what needs to change

[stuartmorgan-g]

Have you run the integration test with the debugger to find out which expectation is failing?

[srivats22]

I did let me try it again and see

[srivats22]

I looked at the errors again

The Linux repo checks failed cause the change log was not correct but I addressed that

the others are failing cause of this:
Unhandled exception:
DriverError: Error while reading FlutterDriver result for command: window.$flutterDriver('{"command":"request_data","timeout":"1200000"}')
Original error: Expected: not null
Actual:

not sure how to proceed

[stuartmorgan-g]

not sure how to proceed

Have you tried running that failing integration test under the debugger, with breakpoints, to find out where exactly the test expectation is failing, as I suggested in my previous comment?

[srivats22]

not sure how to proceed

Have you tried running that failing integration test under the debugger, with breakpoints, to find out where exactly the test expectation is failing, as I suggested in my previous comment?

I have been pasting the below command into my terminal and been running it

dart run script/tool/bin/flutter_plugin_tools.dart drive-examples --packages=google_sign_in/google_sign_in_web --web

and then it runs but gets stuck after sometime

Am i suppose to be running it differently?

[srivats22]

All tests have passed sorry for the delay...

[mdebbar]

LGTM! Thanks for your patience until we got it right :)

[mdebbar]

From web triage: @stuartmorgan-g would you be able to take a final look?