Reject CDP commands when connection closed

[gechnas]

Handle situation when CDP request was send, but response was not received yet. E.g:

• send CDP request (no response yet)
• connection closed

In this case we need to reject CDP command promise (Page.navigate for exampe) instead of waitig forever.

[cyrus-and]

It seems I cannot reproduce the scenario you're describing. For example, if I kill Chrome after a couple of seconds, this script still terminates, printing bye:

const CDP = require('chrome-remote-interface');

(async () => {
    const client = await CDP();

    client.on('disconnect', () => {
        console.log('bye');
    });

    await client.Runtime.evaluate({
        awaitPromise: true,
        expression: `new Promise((x) => setTimeout(x, 5000))`
    });

    await client.close();
})();

[gechnas]

Correct, 'disconnect' event will be emitted - its ok. But my fix is about promise reject. Let's say someone forgot to subscribe for 'disconnect' event:

const CDP = require('chrome-remote-interface');

const delay = ms=>new Promise(resolve=>setTimeout(resolve, ms));

(async () => {
    const client = await CDP();

    // if you close chrome here, before Runtime.evaluate request sent
    // Runtime.evaluate call will throw an error.
    await delay(5000);

    // but if you close chrome right after Runtime.evaluate message sent
    // this promise will not be fulfilled or rejected - this function will stuck forever.
    await client.Runtime.evaluate({
        awaitPromise: true,
        expression: `new Promise((x) => setTimeout(x, 5000))`
    });

    await client.close();
})();

[cyrus-and]

I kind of see your point, but the script terminates whether the disconnect event has been registered or not. If you close Chrome after 1 is printed in the below script, the script still exits, nothing will be stuck forever.

const CDP = require('chrome-remote-interface');

(async () => {
    console.log(0);

    const client = await CDP();

    console.log(1);

    await client.Runtime.evaluate({
        awaitPromise: true,
        expression: `new Promise((x) => setTimeout(x, 5000))`
    });

    console.log(2);

    await client.close();

    console.log(3);
})();

[gechnas]

your'e right. Script will exit in this case if event loop is empty. But I meant that function will stuck, not process. For example, we have http server that running forever, and it need to run some browser job:

app.get('/run/test', async (req, res)=>{
   try {
     const client = await CDP();
     await client.Runtime.evaluate({ // close chrome here
        awaitPromise: true,
        expression: `new Promise((x) => setTimeout(x, 5000))`
     });
   } catch(e){
      return res.status(500).send('Something wrong with browser')
   }
   res.send('done');
});

In this case /test/something request will stuck - 500 error wont be sent.

We actually can solve this problem using 'disconnect' event but in this case we have to write additional logic and handle connection errors in different places:

• inside 'disconnect' event handler - when WS connection was closed after CDP command sent
• inside catch block - when WS connection was closed before CDP command sent

The idea of my PR is that CDP commands should act the same way in different cases: no matter when WS connection was closed (before CDP command was sent or after that) - promise should be rejected.

Yes, it can be fixed using 'disconnect' error, but we have to write additional logic and overcomplicate the code.

[cyrus-and]

This PR has gone too far, cherry-picked the good stuff, and squashed everything in 161cd1768b785336e72fe07141c7eb1a5320a3f2. Thanks!