Promise for request/response with timeout

Question

Below is a piece of code that uses a promise for sending a request to a server, and to wait for an answer. Under normal conditions the server will always respond immediately,

But of course you don't want your code to be stuck forever if a response is missed (e.g. a network issue). So for that reason, it needs a timeout as well.

I usually write this as follows. But it takes quiet a lot of code. Any sugestions ?

new Promise( (resolve, reject) => {
 // the promise is resolved when a predefined response is received.
 const listener = (msg) => {
 const { command } = msg;
 if (command === "pong") resolve();
 };
 try {
 this.addListener(listener);
 // write the command
 this.sendMessage( { command: "ping" });
 // there should be an answer within x seconds
 await new Promise(() => setTimeout(reject, timeout));
 }
 finally {
 this.removeListener(listener);
 }
}

I have been using this as a pattern for nodejs, angular and react code.

• Mast: Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers . Next time, don't leave anything out. Not a good tactic at Code Review.
• bvdb: @Mast, So, yeah, so we leave the bug in it ? - And I make a duplicate question without the bug ? Is that what you are proposing (it seems kind of silly).
• Mast: That wouldn't be a duplicate question, but a follow-up. It might be silly, but the alternative is creating a mess of no-longer applicable answers to multiple states of the same question. We can't have that.

Answer 1

await is only applicable in async functions. The executor function passed to Promise constructor at the code at the question is not defined as async.

The second Promise constructor is not necessary. resolve and reject defined at the single Promise executor can be used.

Include .catch() or function at second parameter of .then() chained to Promise constructor to avoid Uncaught in Promise error and handle potential error within Promise constructor function body.

new Promise((resolve, reject) => {
 try {
 const complete = () => !clearTimeout(time) 
 && this.removeListener(listener);
 const listener = ({command}) => command === "pong" 
 && !complete() && resolve();
 this.addListener(listener);
 this.sendMessage({ command: "ping" });
 const time = setTimeout(() => !complete() && reject(), timeout);
 } catch (e) {
 throw e
 }
})
// handle resolved `Promise`, handle error
.then(() => {}, e => console.error(e));

• bvdb: just for clarity, I also added the wrapping function of which my code was part. That function is indeed marked as async. (I left it out initially for brevity)
• bvdb: Good point about the unnecessary promise that wrapped the setTimeout. - My intent was to make it wait until the timeout was finished. (which isn't stricly necessary, and which wasn't even working properly).