Repository navigation
chore(pubsub): revert port types to reflect numbers and strings - #9549
Conversation
…Options …" This reverts commit 4443d6d.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the port type in ClientConfig to accept both strings and numbers, refactors promise handling, and formats several type definitions. The review feedback correctly identifies a potential issue in the promise chain of closeAllClients_ where a synchronous error thrown by definedCallback(null) would trigger the .catch block, causing the callback to be executed twice. It suggests using the two-argument form of .then() to prevent this.
| .then(() => { | ||
| definedCallback(null); | ||
| }) | ||
| .catch(definedCallback); |
There was a problem hiding this comment.
If definedCallback(null) throws a synchronous error, the promise chain will catch it and invoke definedCallback a second time with the thrown error as the argument. This violates the callback contract (calling it twice) and can lead to unexpected behavior. Using the two-argument form of .then() (similar to the change made in getClient_) ensures that errors thrown inside the success callback do not trigger the error callback.
| .then(() => { | |
| definedCallback(null); | |
| }) | |
| .catch(definedCallback); | |
| .then( | |
| () => { | |
| definedCallback(null); | |
| }, | |
| definedCallback | |
| ) |
References
- Do not wrap callback executions in try-catch blocks to handle synchronous errors without an idempotency guard, as this can cause the callback to be called multiple times if the callback itself throws an error.
quirogas
changed the title
2 participants
Reverts PR #9539 as to not break customers.
The
porttype definition was expanded in PR#3542 as a non-breaking change.