From 4b0b81fd9b17982eb1ce001b1c2a275e983749a6 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Wed, 28 Jun 2017 14:39:37 -0700 Subject: [PATCH] Add better network idle definition (#38) This patch: - Changes network idle promise to wait for 2 or fewer network requests for at least idleTime (defaults to 5s) before resolving. - Adds timer cleanup to failure navigation case. - Adds handling of webSocketClosed. - Ignores unrecognized requestIds to avoid negative inflight requests. References #10 --- lib/Navigator.js | 108 +++++++++++++++++++++++++++++------ test/StaticServer.js | 8 +++ test/assets/networkidle.html | 20 ++++++- test/assets/websocket.html | 28 +++++++++ test/test.js | 67 ++++++++++++++++++++-- 5 files changed, 203 insertions(+), 28 deletions(-) create mode 100644 test/assets/websocket.html diff --git a/lib/Navigator.js b/lib/Navigator.js index c12e8b8c..f09fafea 100644 --- a/lib/Navigator.js +++ b/lib/Navigator.js @@ -14,19 +14,31 @@ * limitations under the License. */ +const VALID_WAIT_CONDITIONS = ['load', 'networkidle']; + class Navigator { /** * @param {!Connection} client * @param {!Object=} options */ - constructor(client, options) { + constructor(client, options = {}) { this._client = client; - client.on('Network.requestWillBeSent', event => this._onRequestWillBeSent(event)); - client.on('Network.loadingFinished', event => this._onLoadingFinished(event)); - client.on('Network.loadingFailed', event => this._onLoadingFailed(event)); - this._minTime = options && options['minTime'] ? options['minTime'] : 0; - this._maxTime = options && options['maxTime'] ? options['maxTime'] : 30000; + this._minTime = typeof options.minTime === 'number' ? options.minTime : 0; + this._maxTime = typeof options.maxTime === 'number' ? options.maxTime : 30000; + this._idleTime = typeof options.networkIdleTimeout === 'number' ? options.networkIdleTimeout : 1000; + this._idleInflight = typeof options.networkIdleInflight === 'number' ? options.networkIdleInflight : 2; + this._waitFor = typeof options.waitFor === 'string' ? options.waitFor : 'load'; this._inflightRequests = 0; + + console.assert(VALID_WAIT_CONDITIONS.includes(this._waitFor)); + + if (this._waitFor === 'networkidle') { + client.on('Network.requestWillBeSent', event => this._onRequestWillBeSent(event)); + client.on('Network.loadingFinished', event => this._onLoadingFinished(event)); + client.on('Network.loadingFailed', event => this._onLoadingFailed(event)); + client.on('Network.webSocketCreated', event => this._onWebSocketCreated(event)); + client.on('Network.webSocketClosed', event => this._onWebSocketClosed(event)); + } } /** @@ -34,29 +46,64 @@ class Navigator { * @param {string=} referrer */ async navigate(url, referrer) { + this._requestIds = new Set(); this._navigationStartTime = Date.now(); - this._watchdogTimer = setTimeout(this._completeNavigation.bind(this, true), this._maxTime); - this._minimumTimer = setTimeout(this._completeNavigation.bind(this, false), this._minTime); - let onload = new Promise(fulfill => this._client.once('Page.loadEventFired', fulfill)); - let networkIdle = new Promise(fulfill => this._navigationLoadCallback = fulfill); - let interstitialPromise = new Promise(fulfill => this._client.once('Security.certificateError', fulfill)).then(() => false); + this._idleReached = false; + + let navigationComplete; + let navigationFailure = new Promise(fulfill => this._client.once('Security.certificateError', fulfill)).then(() => false); + + switch (this._waitFor) { + case 'load': + navigationComplete = new Promise(fulfill => this._client.once('Page.loadEventFired', fulfill)); + break; + case 'networkidle': + navigationComplete = new Promise(fulfill => this._navigationLoadCallback = fulfill); + break; + default: + throw new Error(`Unrecognized wait condition: ${this._waitFor}`); + } this._inflightRequests = 0; + + this._minimumTimer = setTimeout(this._completeNavigation.bind(this, false), this._minTime); + this._maximumTimer = setTimeout(this._completeNavigation.bind(this, true), this._maxTime); + this._idleTimer = setTimeout(this._onIdleReached.bind(this), this._idleTime); + // Await for the command to throw exception in case of illegal arguments. try { await this._client.send('Page.navigate', {url, referrer}); } catch (e) { return false; } - return await Promise.race([Promise.all([onload, networkIdle]).then(() => true), interstitialPromise]); + + return await Promise.race([navigationComplete.then(() => true), navigationFailure]).then(retVal => { + clearTimeout(this._idleTimer); + clearTimeout(this._minimumTimer); + clearTimeout(this._maximumTimer); + return retVal; + }); } /** * @param {!Object} event */ _onRequestWillBeSent(event) { - if (!event.redirectResponse) - ++this._inflightRequests; + this._onLoadingStarted(event); + } + + /** + * @param {!Object} event + */ + _onWebSocketCreated(event) { + this._onLoadingStarted(event); + } + + /** + * @param {!Object} event + */ + _onWebSocketClosed(event) { + this._onLoadingCompleted(event); } /** @@ -73,10 +120,33 @@ class Navigator { this._onLoadingCompleted(event); } + /** + * @param {!Object} event + */ + _onLoadingStarted(event) { + this._requestIds.add(event.requestId); + if (!event.redirectResponse) + ++this._inflightRequests; + if (this._inflightRequests > this._idleInflight) { + clearTimeout(this._idleTimer); + this._idleTimer = null; + } + } + + /** + * @param {!Object} event + */ _onLoadingCompleted(event) { - --this._inflightRequests; - if (Date.now() - this._navigationStartTime < this._minTime) + if (!this._requestIds.has(event.requestId)) return; + + --this._inflightRequests; + if (this._inflightRequests <= this._idleInflight && !this._idleTimer) + this._idleTimer = setTimeout(this._onIdleReached.bind(this), this._idleTime); + } + + _onIdleReached() { + this._idleReached = true; this._completeNavigation(false); } @@ -86,9 +156,9 @@ class Navigator { _completeNavigation(force) { if (!this._navigationLoadCallback) return; - if (this._inflightRequests < 2 || force) { - clearTimeout(this._minimumTimer); - clearTimeout(this._watchdogTimer); + + const elapsedTime = Date.now() - this._navigationStartTime; + if ((elapsedTime >= this._minTime && this._idleReached) || force) { this._navigationLoadCallback(); this._navigationLoadCallback = null; } diff --git a/test/StaticServer.js b/test/StaticServer.js index b7fca60c..de8f9c16 100644 --- a/test/StaticServer.js +++ b/test/StaticServer.js @@ -19,6 +19,7 @@ let url = require('url'); let fs = require('fs'); let path = require('path'); let mime = require('mime'); +let WebSocketServer = require('ws').Server; const fulfillSymbol = Symbol('fullfill callback'); const rejectSymbol = Symbol('reject callback'); @@ -30,6 +31,8 @@ class StaticServer { */ constructor(dirPath, port) { this._server = http.createServer(this._onRequest.bind(this)); + this._wsServer = new WebSocketServer({server: this._server}); + this._wsServer.on('connection', this._onWebSocketConnection.bind(this)); this._server.listen(port); this._dirPath = dirPath; @@ -99,6 +102,7 @@ class StaticServer { if (pathName === '/') pathName = '/index.html'; pathName = path.join(this._dirPath, pathName.substring(1)); + fs.readFile(pathName, function(err, data) { if (err) { response.statusCode = 404; @@ -109,6 +113,10 @@ class StaticServer { response.end(data); }); } + + _onWebSocketConnection(connection) { + connection.send('opened'); + } } module.exports = StaticServer; diff --git a/test/assets/networkidle.html b/test/assets/networkidle.html index c95be2e7..910ae173 100644 --- a/test/assets/networkidle.html +++ b/test/assets/networkidle.html @@ -1,5 +1,19 @@ diff --git a/test/assets/websocket.html b/test/assets/websocket.html new file mode 100644 index 00000000..1a6d2d65 --- /dev/null +++ b/test/assets/websocket.html @@ -0,0 +1,28 @@ + diff --git a/test/test.js b/test/test.js index a9230380..59b650aa 100644 --- a/test/test.js +++ b/test/test.js @@ -115,19 +115,24 @@ describe('Puppeteer', function() { })); it('should wait for network idle to succeed navigation', SX(async function() { let responses = []; - // Hold on a bunch of requests without answering. + // Hold on to a bunch of requests without answering. staticServer.setRoute('/fetch-request-a.js', (req, res) => responses.push(res)); staticServer.setRoute('/fetch-request-b.js', (req, res) => responses.push(res)); staticServer.setRoute('/fetch-request-c.js', (req, res) => responses.push(res)); - let fetchResourcesRequested = Promise.all([ + staticServer.setRoute('/fetch-request-d.js', (req, res) => responses.push(res)); + let initialFetchResourcesRequested = Promise.all([ staticServer.waitForRequest('/fetch-request-a.js'), staticServer.waitForRequest('/fetch-request-b.js'), staticServer.waitForRequest('/fetch-request-c.js'), ]); + let secondFetchResourceRequested = staticServer.waitForRequest('/fetch-request-d.js'); + // Navigate to a page which loads immediately and then does a bunch of // requests via javascript's fetch method. let navigationPromise = page.navigate(STATIC_PREFIX + '/networkidle.html', { - minTime: 50 // Give page time to request more resources dynamically. + waitFor: 'networkidle', + networkIdleTimeout: 100, + networkIdleInflight: 0, // Only be idle when there are 0 inflight requests }); // Track when the navigation gets completed. let navigationFinished = false; @@ -137,13 +142,63 @@ describe('Puppeteer', function() { await new Promise(fulfill => page.once('load', fulfill)); expect(navigationFinished).toBe(false); - // Wait for all three resources to be requested. - await fetchResourcesRequested; + // Wait for the initial three resources to be requested. + await initialFetchResourcesRequested; // Expect navigation still to be not finished. expect(navigationFinished).toBe(false); - // Respond to all requests. + // Respond to initial requests. + for (let response of responses) { + response.statusCode = 404; + response.end(`File not found`); + } + + // Reset responses array + responses = []; + + // Wait for the second round to be requested. + await secondFetchResourceRequested; + // Expect navigation still to be not finished. + expect(navigationFinished).toBe(false); + + // Respond to requests. + for (let response of responses) { + response.statusCode = 404; + response.end(`File not found`); + } + + let success = await navigationPromise; + // Expect navigation to succeed. + expect(success).toBe(true); + })); + it('should wait for websockets to succeed navigation', SX(async function() { + let responses = []; + // Hold on to the fetch request without answering. + staticServer.setRoute('/fetch-request.js', (req, res) => responses.push(res)); + let fetchResourceRequested = staticServer.waitForRequest('/fetch-request.js'); + // Navigate to a page which loads immediately and then opens a bunch of + // websocket connections and then a fetch request. + let navigationPromise = page.navigate(STATIC_PREFIX + '/websocket.html', { + waitFor: 'networkidle', + networkIdleTimeout: 100, + networkIdleInflight: 0, // Only be idle when there are 0 inflight requests/connections + }); + // Track when the navigation gets completed. + let navigationFinished = false; + navigationPromise.then(() => navigationFinished = true); + + // Wait for the page's 'load' event. + await new Promise(fulfill => page.once('load', fulfill)); + expect(navigationFinished).toBe(false); + + // Wait for the resource to be requested. + await fetchResourceRequested; + + // Expect navigation still to be not finished. + expect(navigationFinished).toBe(false); + + // Respond to the request. for (let response of responses) { response.statusCode = 404; response.end(`File not found`);