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

fix: make async iteration work for gRPC-fallback; refactor the code #765

Merged
merged 2 commits into from
Mar 28, 2020

Conversation

alexander-fenster
Copy link
Contributor

TL;DR: this is a small refactor of the client functionality that was not released yet. Pretty safe to merge since async iteration in clients is not enabled yet.


So this turned to be a refactor that made the async iteration code a little bit simpler.

Background: it turns out async iteration, as implemented, did not work for gRPC-fallback clients. The main reason for that is that createApiCall is implemented differently in fallback.ts, and fallback clients expect that exact implementation. Long story short, it was not possible to call a stub of pagedExpand in pagedExpandAsync; fallback clients needed the result of createApiCall.

So... since the result of createApiCall is a function and not a promise, we can remove all the complicated asynchronous stuff (uninitialized promises, etc.) from the async iterator implementation, and make it pretty much straightforward by reusing the paging logic already implemented elsewhere (so I'm just disabling auto-pagination with autoPaginate: false and reusing the regular paginator).

The corresponding client change is coming as well (see the test client change to get an idea how the client is changed).

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 28, 2020
@codecov
Copy link

codecov bot commented Mar 28, 2020

Codecov Report

Merging #765 into master will decrease coverage by 0.03%.
The diff coverage is 94.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #765      +/-   ##
==========================================
- Coverage   89.47%   89.43%   -0.04%     
==========================================
  Files          45       45              
  Lines        7476     7433      -43     
  Branches      481      457      -24     
==========================================
- Hits         6689     6648      -41     
+ Misses        784      782       -2     
  Partials        3        3              
Impacted Files Coverage Δ
src/apitypes.ts 0.00% <0.00%> (ø)
src/paginationCalls/pageDescriptor.ts 90.24% <95.45%> (+8.13%) ⬆️
samples/pagination.js 100.00% <100.00%> (ø)
...ixtures/google-gax-packaging-test-app/src/index.js 94.88% <100.00%> (+0.02%) ⬆️
...-gax-packaging-test-app/src/v1beta1/echo_client.js 98.16% <100.00%> (ø)
src/normalCalls/retries.ts 93.58% <0.00%> (-5.13%) ⬇️
src/fallback.ts 74.21% <0.00%> (-2.90%) ⬇️
src/streamingCalls/streamingApiCaller.ts 89.24% <0.00%> (-1.08%) ⬇️
src/grpc.ts 89.14% <0.00%> (-0.58%) ⬇️
src/gax.ts 96.39% <0.00%> (+0.30%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72114db...e864c8f. Read the comment docs.

Copy link
Contributor

@xiaozhenliu-gg5 xiaozhenliu-gg5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for debugging and fixing this!

One question though: what does that mean by grpc-fallback expects the "exact same" implementation of the apiCall? I noticed the apiCall in unit test is changed to apiCall instead of apiCallPromise, but the way how to call async iterator stays the same? Thank you!

}

makeCall(request: RequestType, func: GaxCall, settings: CallSettings) {
if (settings.pageToken) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious that if we remove this block, does setting includes all pagetoken/pageSize information?

throw new Error(error);
});
options = Object.assign({}, options, {autoPaginate: false});
const iterable = this.createIterator(apiCall, request, options);
return iterable;
Copy link
Contributor

@xiaozhenliu-gg5 xiaozhenliu-gg5 Mar 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So here we don't have to wait for params to resolve, when asyncIterate is called, we have the apiCall and needed request, options, is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because apiCall is already a function and everything is synchronous.

@alexander-fenster
Copy link
Contributor Author

The problem with fallback code is that it uses a different way to communicate with the server. gRPC parses proto files and provides its own stubs that we access by createStub and decorate by calling createApiCall, but fallback uses Protobuf.js rpcImpl described here. Protobuf.js expects rpcImpl to have callback interface, and callback interface is provided by createApiCall. So in the original code, when innerCallPromise[method] got called, no callback was passed to rpcImpl and it failed with a weird error:

  1) Showcase tests
       grpc-fallback works
         pagedExpandAsync:
     TypeError: undefined is not a function
      at Service.newServiceStub.<computed> [as pagedExpand] (node_modules/google-gax/build/src/fallback.js:187:64)
      at /Users/fenster/gapic-generator-typescript/.test-application-js/node_modules/showcase/build/src/v1beta1/echo_client.js:197:29
      at OngoingCallPromise.call (node_modules/google-gax/build/src/call.js:67:27)
      at PageDescriptor.getNextPageRequest (node_modules/google-gax/build/src/paginationCalls/pageDescriptor.js:141:21)
      at Object.next (node_modules/google-gax/build/src/paginationCalls/pageDescriptor.js:127:54)
      at async Context.<anonymous> (index.js:113:22)

TypeError: undefined is not a function occurs because a callback is not passed to rpcImpl and it tries to call an undefined function. The easiest fix was to switch to using apiCall instead of innerCallPromise, and it turns out it allows easier (synchronous) implementation.

^^^ This might be complicated, I spent about an hour trying to understand what's going on :)

@alexander-fenster alexander-fenster merged commit 944c06b into master Mar 28, 2020
@alexander-fenster alexander-fenster deleted the fix-async-iterator branch March 28, 2020 05:19
gcf-merge-on-green bot pushed a commit that referenced this pull request Mar 28, 2020
yoshi-automation added a commit that referenced this pull request Apr 1, 2020
…765)

944c06b
commit 944c06b
Author: Alexander Fenster <fenster@google.com>
Date:   Fri Mar 27 22:19:15 2020 -0700

    fix: make async iteration work for gRPC-fallback; refactor the code (#765)
yoshi-automation added a commit that referenced this pull request Apr 1, 2020
3ce902d
commit 3ce902d
Author: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Date:   Sat Mar 28 05:30:07 2020 +0000

    chore: release 2.0.1 (#766)

    🤖 I have created a release \*beep\* \*boop\*
    ---
    ### [2.0.1](https://www.github.com/googleapis/gax-nodejs/compare/v2.0.0...v2.0.1) (2020-03-28)

    ### Bug Fixes

    * **deps:** update dependency google-auth-library to v6 ([#763](https://www.github.com/googleapis/gax-nodejs/issues/763)) ([72114db](https://www.github.com/googleapis/gax-nodejs/commit/72114db1b15af3034c51a784e3fc619e2ee281e5))
    * make async iteration work for gRPC-fallback; refactor the code ([#765](https://www.github.com/googleapis/gax-nodejs/issues/765)) ([944c06b](https://www.github.com/googleapis/gax-nodejs/commit/944c06b4225c4dd5cdcf08e4ca2497cfe3a69cde))
    ---

    This PR was generated with [Release Please](https://github.com/googleapis/release-please).
alexander-fenster pushed a commit that referenced this pull request Apr 1, 2020
* chore: use gts v2 (#757)

247aeb7
commit 247aeb7
Author: Alexander Fenster <fenster@google.com>
Date:   Thu Mar 26 13:04:10 2020 -0700

    chore: use gts v2 (#757)

    * chore: use gts v2

    * chore: ahhhhh thaaaats why

    * chore: do not run on node8

    * fix: new gts rules

* build: use TypeScript ^3.8.3, set lib to es2018 (#760)

41c73f8
commit 41c73f8
Author: Alexander Fenster <fenster@google.com>
Date:   Thu Mar 26 13:34:19 2020 -0700

    build: use TypeScript ^3.8.3, set lib to es2018 (#760)

    * build: use TypeScript ^3.8.3, set lib to es2018

    * fix: better typing

* chore!: require Node.js v10+ (#759)

23ec7f6
commit 23ec7f6
Author: Alexander Fenster <fenster@google.com>
Date:   Thu Mar 26 14:07:14 2020 -0700

    chore!: require Node.js v10+ (#759)

    BREAKING CHANGE: use Node.js v10+

* chore(deps): update dependency @types/rimraf to v3 (#751)

2f1b6ae
commit 2f1b6ae
Author: WhiteSource Renovate <bot@renovateapp.com>
Date:   Thu Mar 26 22:27:20 2020 +0100

    chore(deps): update dependency @types/rimraf to v3 (#751)

    Co-authored-by: Alexander Fenster <fenster@google.com>

* chore: release 2.0.0 (#738)

1c11c80
commit 1c11c80
Author: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Date:   Thu Mar 26 22:32:20 2020 +0000

    chore: release 2.0.0 (#738)

    🤖 I have created a release \*beep\* \*boop\*
    ---
    ## [2.0.0](https://www.github.com/googleapis/gax-nodejs/compare/v1.14.2...v2.0.0) (2020-03-26)

    ### ⚠ BREAKING CHANGES

    * use Node.js v10+
    * throw for versions of Node.js older than v10.0.0 (#748)
    * stop accepting Promise constructor (#737)

    ### Features

    * export bundle descriptor in descriptors interface ([#744](https://www.github.com/googleapis/gax-nodejs/issues/744)) ([b1eccf9](https://www.github.com/googleapis/gax-nodejs/commit/b1eccf96c439d67376d249a54c9d22ffe7ff1839))
    * export ServiceError from @grpc/grpc-js ([#754](https://www.github.com/googleapis/gax-nodejs/issues/754)) ([24a4d60](https://www.github.com/googleapis/gax-nodejs/commit/24a4d600738a9597e9a87d6705eaed3dc2285e3b))
    * stop accepting Promise constructor ([#737](https://www.github.com/googleapis/gax-nodejs/issues/737)) ([816bf9b](https://www.github.com/googleapis/gax-nodejs/commit/816bf9b283217208debd979e893a6daf29f1f739))
    * support async iterator for paging method ([#708](https://www.github.com/googleapis/gax-nodejs/issues/708)) ([3ac5afb](https://www.github.com/googleapis/gax-nodejs/commit/3ac5afb3b1b1b22798f15ee07395f3ca765383b4))
    * throw for versions of Node.js older than v10.0.0 ([#748](https://www.github.com/googleapis/gax-nodejs/issues/748)) ([511fc23](https://www.github.com/googleapis/gax-nodejs/commit/511fc233bd66d537c24743ef460ee8c609cd551f))

    ### Bug Fixes

    * **deps:** update dependency @grpc/grpc-js to ^0.7.0 ([#736](https://www.github.com/googleapis/gax-nodejs/issues/736)) ([01c428c](https://www.github.com/googleapis/gax-nodejs/commit/01c428cb1240320b92778abf1297a5ff72346fd9))
    * **deps:** use @grpc/grpc-js v0.7.2 ([#735](https://www.github.com/googleapis/gax-nodejs/issues/735)) ([836e81b](https://www.github.com/googleapis/gax-nodejs/commit/836e81b64f84d8c118e6aea0580f0645658a8490))
    * **deps:** use protobuf.js v6.8.9 ([#743](https://www.github.com/googleapis/gax-nodejs/issues/743)) ([fab91ce](https://www.github.com/googleapis/gax-nodejs/commit/fab91ce334d76212d7e31b5478331339c5acad76))
    * allow passing numbers as path template parameters ([#756](https://www.github.com/googleapis/gax-nodejs/issues/756)) ([c466d3d](https://www.github.com/googleapis/gax-nodejs/commit/c466d3dc68c8f9050d3ae69dcedd708e3509ae17))

    ### Miscellaneous Chores

    * require Node.js v10+ ([#759](https://www.github.com/googleapis/gax-nodejs/issues/759)) ([23ec7f6](https://www.github.com/googleapis/gax-nodejs/commit/23ec7f69c3813f6d06ea8b2a473d072337e1b499))
    ---

    This PR was generated with [Release Please](https://github.com/googleapis/release-please).

* fix(deps): update dependency google-auth-library to v6 (#763)

72114db
commit 72114db
Author: WhiteSource Renovate <bot@renovateapp.com>
Date:   Sat Mar 28 04:08:38 2020 +0100

    fix(deps): update dependency google-auth-library to v6 (#763)

* fix: make async iteration work for gRPC-fallback; refactor the code (#765)

944c06b
commit 944c06b
Author: Alexander Fenster <fenster@google.com>
Date:   Fri Mar 27 22:19:15 2020 -0700

    fix: make async iteration work for gRPC-fallback; refactor the code (#765)

* chore: release 2.0.1 (#766)

3ce902d
commit 3ce902d
Author: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Date:   Sat Mar 28 05:30:07 2020 +0000

    chore: release 2.0.1 (#766)

    🤖 I have created a release \*beep\* \*boop\*
    ---
    ### [2.0.1](https://www.github.com/googleapis/gax-nodejs/compare/v2.0.0...v2.0.1) (2020-03-28)

    ### Bug Fixes

    * **deps:** update dependency google-auth-library to v6 ([#763](https://www.github.com/googleapis/gax-nodejs/issues/763)) ([72114db](https://www.github.com/googleapis/gax-nodejs/commit/72114db1b15af3034c51a784e3fc619e2ee281e5))
    * make async iteration work for gRPC-fallback; refactor the code ([#765](https://www.github.com/googleapis/gax-nodejs/issues/765)) ([944c06b](https://www.github.com/googleapis/gax-nodejs/commit/944c06b4225c4dd5cdcf08e4ca2497cfe3a69cde))
    ---

    This PR was generated with [Release Please](https://github.com/googleapis/release-please).

* build: set AUTOSYNTH_MULTIPLE_COMMITS=true for context aware commits (#770)

a1e4a3f
commit a1e4a3f
Author: Benjamin E. Coe <bencoe@google.com>
Date:   Tue Mar 31 18:34:30 2020 -0700

    build: set AUTOSYNTH_MULTIPLE_COMMITS=true for context aware commits (#770)

* fix: do not run node 8 CI (#456)

googleapis/synthtool@1b4cc80
commit 1b4cc80a7aaf164f6241937dd87f3bd1f4149e0c
Author: Alexander Fenster <fenster@google.com>
Date:   Wed Mar 25 08:01:31 2020 -0700

    fix: do not run node 8 CI (#456)

* fix: update template files for Node.js libraries (#463)

googleapis/synthtool@9982024
commit 99820243d348191bc9c634f2b48ddf65096285ed
Author: Alexander Fenster <fenster@google.com>
Date:   Tue Mar 31 11:56:27 2020 -0700

    fix: update template files for Node.js libraries (#463)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants