-
Notifications
You must be signed in to change notification settings - Fork 24
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
feat: add close() API #126
Changes from 2 commits
8dba01a
731df7a
e2a042b
4b8b101
cf0b1ff
fd60a63
5dbebdd
3e5b8c2
d54fd7d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -49,6 +49,7 @@ export interface PaginationResponse< | |||||
*/ | ||||||
export class EchoClient { | ||||||
private _descriptors: Descriptors = {page: {}, stream: {}, longrunning: {}}; | ||||||
private _echoStub: ClientStub; | ||||||
private _innerApiCalls: {[name: string]: Function}; | ||||||
auth: gax.GoogleAuth; | ||||||
|
||||||
|
@@ -191,7 +192,7 @@ export class EchoClient { | |||||
|
||||||
// Put together the "service stub" for | ||||||
// google.showcase.v1beta1.Echo. | ||||||
const echoStub = gaxGrpc.createStub( | ||||||
this._echoStub = gaxGrpc.createStub( | ||||||
opts.fallback ? | ||||||
(protos as protobuf.Root).lookupService('google.showcase.v1beta1.Echo') : | ||||||
// tslint:disable-next-line no-any | ||||||
|
@@ -204,21 +205,28 @@ export class EchoClient { | |||||
['echo', 'expand', 'collect', 'chat', 'pagedExpand', 'wait']; | ||||||
|
||||||
for (const methodName of echoStubMethods) { | ||||||
const innerCallPromise = echoStub.then( | ||||||
const innerCallPromise = this._echoStub.then( | ||||||
(stub: {[method: string]: Function}) => (...args: Array<{}>) => { | ||||||
return stub[methodName].apply(stub, args); | ||||||
}, | ||||||
(err: Error|null|undefined) => () => { | ||||||
throw err; | ||||||
}); | ||||||
|
||||||
this._innerApiCalls[methodName] = gaxModule.createApiCall( | ||||||
const apiCall = gaxModule.createApiCall( | ||||||
innerCallPromise, | ||||||
defaults[methodName], | ||||||
this._descriptors.page[methodName] || | ||||||
this._descriptors.stream[methodName] || | ||||||
this._descriptors.stream[methodName] || | ||||||
this._descriptors.longrunning[methodName] | ||||||
); | ||||||
|
||||||
this._innerApiCalls[methodName] = (arguments, callOptions, callback) => { | ||||||
if (!this._terminated) { | ||||||
return Promise.reject('The client has already been closed.'); | ||||||
} | ||||||
return apiCall(arguments, callOptions, callback); | ||||||
}; | ||||||
} | ||||||
} | ||||||
|
||||||
|
@@ -578,4 +586,16 @@ export class EchoClient { | |||||
callSettings | ||||||
); | ||||||
} | ||||||
|
||||||
/** | ||||||
* Terminate the GRPC channel and close the client. | ||||||
* | ||||||
* The client will no longer be usable and all future behavior is undefined. | ||||||
*/ | ||||||
close(): void { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like we should return a Promise here, so that users can await it?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The method we are invoking is synchronous: https://github.com/grpc/grpc-node/blob/master/packages/grpc-js/src/client.ts#L90 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, I was hoping close() could be synchronous, but you are right. I updated the PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately, due to the asynchronous nature of the constructor (it fires some promises to initialize the stubs), all member functions working with a stub must be asynchronous (must wait on the stubs to be complete). |
||||||
if (!this._terminated) { | ||||||
this._terminated = true; | ||||||
this._echoStub.then(stub => stub.close()); | ||||||
} | ||||||
} | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like the condition must be reversed