From 5de96c373d770cdf3eb1f008d69eb79e510d6515 Mon Sep 17 00:00:00 2001 From: picklesrus Date: Mon, 6 Jul 2020 17:44:28 -0400 Subject: [PATCH 1/7] Exponentially back off the time between message polling instead of doing it every two minutes. --- src/components/navigation/www/navigation.jsx | 39 +++++---- test/unit/components/navigation.test.jsx | 87 +++++++++++++++++++- 2 files changed, 110 insertions(+), 16 deletions(-) diff --git a/src/components/navigation/www/navigation.jsx b/src/components/navigation/www/navigation.jsx index 515b29b82..9ed2e3cb4 100644 --- a/src/components/navigation/www/navigation.jsx +++ b/src/components/navigation/www/navigation.jsx @@ -27,7 +27,8 @@ class Navigation extends React.Component { super(props); bindAll(this, [ 'getProfileUrl', - 'handleSearchSubmit' + 'handleSearchSubmit', + 'setupMessagePolling' ]); this.state = { messageCountIntervalId: -1 // javascript method interval id for getting messsage count. @@ -35,27 +36,18 @@ class Navigation extends React.Component { } componentDidMount () { if (this.props.user) { - const intervalId = setInterval(() => { - this.props.getMessageCount(this.props.user.username); - }, 120000); // check for new messages every 2 mins. - this.setState({ // eslint-disable-line react/no-did-mount-set-state - messageCountIntervalId: intervalId - }); + // Setup polling for messages to start in 2 minutes. + setTimeout(this.setupMessagePolling.bind(this, 2), 2 * 60 * 1000); } } componentDidUpdate (prevProps) { if (prevProps.user !== this.props.user) { this.props.handleCloseAccountNav(); if (this.props.user) { - const intervalId = setInterval(() => { - this.props.getMessageCount(this.props.user.username); - }, 120000); // check for new messages every 2 mins. - this.setState({ // eslint-disable-line react/no-did-update-set-state - messageCountIntervalId: intervalId - }); + setTimeout(this.setupMessagePolling.bind(this, 2), 2 * 60 * 1000); } else { // clear message count check, and set to default id. - clearInterval(this.state.messageCountIntervalId); + clearTimeout(this.state.messageCountIntervalId); this.props.setMessageCount(0); this.setState({ // eslint-disable-line react/no-did-update-set-state messageCountIntervalId: -1 @@ -66,7 +58,7 @@ class Navigation extends React.Component { componentWillUnmount () { // clear message interval if it exists if (this.state.messageCountIntervalId !== -1) { - clearInterval(this.state.messageCountIntervalId); + clearTimeout(this.state.messageCountIntervalId); this.props.setMessageCount(0); this.setState({ messageCountIntervalId: -1 @@ -77,6 +69,23 @@ class Navigation extends React.Component { if (!this.props.user) return; return `/users/${this.props.user.username}/`; } + + setupMessagePolling (minutes) { + this.props.getMessageCount(this.props.user.username); + // We only poll if it has been less than 30 minutes. + // Chances of someone actively using the page for that long without + // a navigation is low. + if (minutes < 32) { + const nextFetch = minutes * 2; + const timeoutId = setTimeout(() => { + this.setupMessagePolling(nextFetch); + }, nextFetch * 60000); + this.setState({ // eslint-disable-line react/no-did-mount-set-state + messageCountIntervalId: timeoutId + }); + } + } + handleSearchSubmit (formData) { let targetUrl = '/search/projects'; if (formData.q) { diff --git a/test/unit/components/navigation.test.jsx b/test/unit/components/navigation.test.jsx index 9a9547a9a..e74bf125b 100644 --- a/test/unit/components/navigation.test.jsx +++ b/test/unit/components/navigation.test.jsx @@ -1,5 +1,5 @@ const React = require('react'); -const {shallowWithIntl} = require('../../helpers/intl-helpers.jsx'); +const {shallowWithIntl, mountWithIntl} = require('../../helpers/intl-helpers.jsx'); import configureStore from 'redux-mock-store'; const Navigation = require('../../../src/components/navigation/www/navigation.jsx'); const Registration = require('../../../src/components/registration/registration.jsx'); @@ -11,6 +11,7 @@ describe('Navigation', () => { beforeEach(() => { store = null; + jest.useFakeTimers(); }); const getNavigationWrapper = props => { @@ -103,4 +104,88 @@ describe('Navigation', () => { navWrapper.find('a.registrationLink').simulate('click', {preventDefault () {}}); expect(navInstance.props.handleClickRegistration).toHaveBeenCalled(); }); + + test('Component sets up message polling when it mounts', () => { + store = mockStore({ + navigation: { + registrationOpen: false + }, + messageCount: { + messageCount: 5 + } + }); + const props = { + user: { + thumbnailUrl: 'scratch.mit.edu', + username: 'auser' + }, + getMessageCount: jest.fn() + }; + const intlWrapper = mountWithIntl( + , {context: {store}, + childContextTypes: {store} + }); + + intlWrapper.children().find('Navigation') + .instance(); + const twoMin = 2 * 60 * 1000; + expect(setTimeout).toHaveBeenLastCalledWith(expect.any(Function), twoMin); + // Advance timers passed the intial two minutes. + jest.advanceTimersByTime(twoMin + 1); + expect(setTimeout).toHaveBeenLastCalledWith(expect.any(Function), twoMin * 2); + expect(props.getMessageCount).toHaveBeenCalled(); + }); + + test('SetupMessagePolling polls for messages 5 times', () => { + store = mockStore({ + navigation: { + registrationOpen: false + }, + messageCount: { + messageCount: 5 + } + }); + const props = { + user: { + thumbnailUrl: 'scratch.mit.edu', + username: 'auser' + }, + getMessageCount: jest.fn() + }; + const intlWrapper = mountWithIntl( + , {context: {store}, + childContextTypes: {store} + }); + + const navInstance = intlWrapper.children().find('Navigation') + .instance(); + // Clear the timers and mocks because componentDidMount + // has already called setupMessagePolling. + jest.clearAllTimers(); + jest.clearAllMocks(); + + navInstance.setupMessagePolling(2); + + // Check that we set the timeout to backoff exponentially + let minutes = 2 * 60 * 1000; + for (let count = 1; count < 5; ++count) { + jest.advanceTimersByTime(minutes + 1); + expect(setTimeout).toHaveBeenLastCalledWith(expect.any(Function), minutes * 2); + expect(props.getMessageCount).toHaveBeenCalledTimes(count); + minutes = minutes * 2; + } + + // Exhaust all timers (there shouldn't be any left) + jest.runAllTimers(); + // We exponentially back off checking for messages, starting at 2 min + // and stop after 32 minutes so it should happen 5 times total. + expect(props.getMessageCount).toHaveBeenCalledTimes(5); + // setTimeout happens 1 fewer since the original call to + // setupMessagePolling isn't counted here. + expect(setTimeout).toHaveBeenCalledTimes(4); + }); }); From 7eeb63cb3f887770b68c36cd56e60ba390eea128 Mon Sep 17 00:00:00 2001 From: picklesrus Date: Thu, 16 Jul 2020 12:24:13 -0400 Subject: [PATCH 2/7] rename SetupMessagePolling to pollForMessages --- src/components/navigation/www/navigation.jsx | 10 +++++----- test/unit/components/navigation.test.jsx | 8 ++++---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/components/navigation/www/navigation.jsx b/src/components/navigation/www/navigation.jsx index 9ed2e3cb4..3346aeb79 100644 --- a/src/components/navigation/www/navigation.jsx +++ b/src/components/navigation/www/navigation.jsx @@ -28,7 +28,7 @@ class Navigation extends React.Component { bindAll(this, [ 'getProfileUrl', 'handleSearchSubmit', - 'setupMessagePolling' + 'pollForMessages' ]); this.state = { messageCountIntervalId: -1 // javascript method interval id for getting messsage count. @@ -37,14 +37,14 @@ class Navigation extends React.Component { componentDidMount () { if (this.props.user) { // Setup polling for messages to start in 2 minutes. - setTimeout(this.setupMessagePolling.bind(this, 2), 2 * 60 * 1000); + setTimeout(this.pollForMessages.bind(this, 2), 2 * 60 * 1000); } } componentDidUpdate (prevProps) { if (prevProps.user !== this.props.user) { this.props.handleCloseAccountNav(); if (this.props.user) { - setTimeout(this.setupMessagePolling.bind(this, 2), 2 * 60 * 1000); + setTimeout(this.pollForMessages.bind(this, 2), 2 * 60 * 1000); } else { // clear message count check, and set to default id. clearTimeout(this.state.messageCountIntervalId); @@ -70,7 +70,7 @@ class Navigation extends React.Component { return `/users/${this.props.user.username}/`; } - setupMessagePolling (minutes) { + pollForMessages (minutes) { this.props.getMessageCount(this.props.user.username); // We only poll if it has been less than 30 minutes. // Chances of someone actively using the page for that long without @@ -78,7 +78,7 @@ class Navigation extends React.Component { if (minutes < 32) { const nextFetch = minutes * 2; const timeoutId = setTimeout(() => { - this.setupMessagePolling(nextFetch); + this.pollForMessages(nextFetch); }, nextFetch * 60000); this.setState({ // eslint-disable-line react/no-did-mount-set-state messageCountIntervalId: timeoutId diff --git a/test/unit/components/navigation.test.jsx b/test/unit/components/navigation.test.jsx index e74bf125b..2333bc509 100644 --- a/test/unit/components/navigation.test.jsx +++ b/test/unit/components/navigation.test.jsx @@ -138,7 +138,7 @@ describe('Navigation', () => { expect(props.getMessageCount).toHaveBeenCalled(); }); - test('SetupMessagePolling polls for messages 5 times', () => { + test('pollForMessages polls for messages 5 times', () => { store = mockStore({ navigation: { registrationOpen: false @@ -164,11 +164,11 @@ describe('Navigation', () => { const navInstance = intlWrapper.children().find('Navigation') .instance(); // Clear the timers and mocks because componentDidMount - // has already called setupMessagePolling. + // has already called pollForMessages. jest.clearAllTimers(); jest.clearAllMocks(); - navInstance.setupMessagePolling(2); + navInstance.pollForMessages(2); // Check that we set the timeout to backoff exponentially let minutes = 2 * 60 * 1000; @@ -185,7 +185,7 @@ describe('Navigation', () => { // and stop after 32 minutes so it should happen 5 times total. expect(props.getMessageCount).toHaveBeenCalledTimes(5); // setTimeout happens 1 fewer since the original call to - // setupMessagePolling isn't counted here. + // pollForMessages isn't counted here. expect(setTimeout).toHaveBeenCalledTimes(4); }); }); From fac3ccad5799df4d4f74298ca60d0415a5aee8bb Mon Sep 17 00:00:00 2001 From: picklesrus Date: Fri, 17 Jul 2020 14:44:36 -0400 Subject: [PATCH 3/7] Make all times be in ms. --- src/components/navigation/www/navigation.jsx | 14 ++++++++------ test/unit/components/navigation.test.jsx | 12 ++++++------ 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/src/components/navigation/www/navigation.jsx b/src/components/navigation/www/navigation.jsx index 3346aeb79..4844fe8ca 100644 --- a/src/components/navigation/www/navigation.jsx +++ b/src/components/navigation/www/navigation.jsx @@ -37,14 +37,16 @@ class Navigation extends React.Component { componentDidMount () { if (this.props.user) { // Setup polling for messages to start in 2 minutes. - setTimeout(this.pollForMessages.bind(this, 2), 2 * 60 * 1000); + const twoMinInMs = 2 * 60 * 1000; + setTimeout(this.pollForMessages.bind(this, twoMinInMs), twoMinInMs); } } componentDidUpdate (prevProps) { if (prevProps.user !== this.props.user) { this.props.handleCloseAccountNav(); if (this.props.user) { - setTimeout(this.pollForMessages.bind(this, 2), 2 * 60 * 1000); + const twoMinInMs = 2 * 60 * 1000; + setTimeout(this.pollForMessages.bind(this, twoMinInMs), twoMinInMs); } else { // clear message count check, and set to default id. clearTimeout(this.state.messageCountIntervalId); @@ -70,16 +72,16 @@ class Navigation extends React.Component { return `/users/${this.props.user.username}/`; } - pollForMessages (minutes) { + pollForMessages (ms) { this.props.getMessageCount(this.props.user.username); // We only poll if it has been less than 30 minutes. // Chances of someone actively using the page for that long without // a navigation is low. - if (minutes < 32) { - const nextFetch = minutes * 2; + if (ms < 32 * 60 * 1000) { // 32 minutes + const nextFetch = ms * 2; // exponentially back off next fetch time. const timeoutId = setTimeout(() => { this.pollForMessages(nextFetch); - }, nextFetch * 60000); + }, nextFetch); this.setState({ // eslint-disable-line react/no-did-mount-set-state messageCountIntervalId: timeoutId }); diff --git a/test/unit/components/navigation.test.jsx b/test/unit/components/navigation.test.jsx index 2333bc509..07c8a7139 100644 --- a/test/unit/components/navigation.test.jsx +++ b/test/unit/components/navigation.test.jsx @@ -167,16 +167,16 @@ describe('Navigation', () => { // has already called pollForMessages. jest.clearAllTimers(); jest.clearAllMocks(); - - navInstance.pollForMessages(2); + let twoMinInMs = 2 * 60 * 1000; // 2 minutes in ms. + navInstance.pollForMessages(twoMinInMs); // Check that we set the timeout to backoff exponentially - let minutes = 2 * 60 * 1000; + for (let count = 1; count < 5; ++count) { - jest.advanceTimersByTime(minutes + 1); - expect(setTimeout).toHaveBeenLastCalledWith(expect.any(Function), minutes * 2); + jest.advanceTimersByTime(twoMinInMs + 1); + expect(setTimeout).toHaveBeenLastCalledWith(expect.any(Function), twoMinInMs * 2); expect(props.getMessageCount).toHaveBeenCalledTimes(count); - minutes = minutes * 2; + twoMinInMs = twoMinInMs * 2; } // Exhaust all timers (there shouldn't be any left) From 0036550ae8c4e5ab808ba6f82ec674a475e09a67 Mon Sep 17 00:00:00 2001 From: picklesrus Date: Thu, 30 Jul 2020 10:55:18 -0400 Subject: [PATCH 4/7] Move timeout id out of state to a member variable and add some unittests. --- src/components/navigation/www/navigation.jsx | 27 +++++-------- test/unit/components/navigation.test.jsx | 40 +++++++++++++++++++- 2 files changed, 48 insertions(+), 19 deletions(-) diff --git a/src/components/navigation/www/navigation.jsx b/src/components/navigation/www/navigation.jsx index 4844fe8ca..81c83681e 100644 --- a/src/components/navigation/www/navigation.jsx +++ b/src/components/navigation/www/navigation.jsx @@ -30,15 +30,14 @@ class Navigation extends React.Component { 'handleSearchSubmit', 'pollForMessages' ]); - this.state = { - messageCountIntervalId: -1 // javascript method interval id for getting messsage count. - }; + // Keep the timeout id so we can cancel it (e.g. when we unmount) + this.messageCountTimeoutId = -1; } componentDidMount () { if (this.props.user) { // Setup polling for messages to start in 2 minutes. const twoMinInMs = 2 * 60 * 1000; - setTimeout(this.pollForMessages.bind(this, twoMinInMs), twoMinInMs); + this.messageCountTimeoutId = setTimeout(this.pollForMessages.bind(this, twoMinInMs), twoMinInMs); } } componentDidUpdate (prevProps) { @@ -46,25 +45,21 @@ class Navigation extends React.Component { this.props.handleCloseAccountNav(); if (this.props.user) { const twoMinInMs = 2 * 60 * 1000; - setTimeout(this.pollForMessages.bind(this, twoMinInMs), twoMinInMs); + this.messageCountTimeoutId = setTimeout(this.pollForMessages.bind(this, twoMinInMs), twoMinInMs); } else { // clear message count check, and set to default id. - clearTimeout(this.state.messageCountIntervalId); + clearTimeout(this.messageCountTimeoutId); this.props.setMessageCount(0); - this.setState({ // eslint-disable-line react/no-did-update-set-state - messageCountIntervalId: -1 - }); + this.messageCountTimeoutId = -1; } } } componentWillUnmount () { // clear message interval if it exists - if (this.state.messageCountIntervalId !== -1) { - clearTimeout(this.state.messageCountIntervalId); + if (this.messageCountTimeoutId !== -1) { + clearTimeout(this.messageCountTimeoutId); this.props.setMessageCount(0); - this.setState({ - messageCountIntervalId: -1 - }); + this.messageCountTimeoutId = -1; } } getProfileUrl () { @@ -82,9 +77,7 @@ class Navigation extends React.Component { const timeoutId = setTimeout(() => { this.pollForMessages(nextFetch); }, nextFetch); - this.setState({ // eslint-disable-line react/no-did-mount-set-state - messageCountIntervalId: timeoutId - }); + this.messageCountTimeoutId = timeoutId; } } diff --git a/test/unit/components/navigation.test.jsx b/test/unit/components/navigation.test.jsx index 07c8a7139..fa180f63a 100644 --- a/test/unit/components/navigation.test.jsx +++ b/test/unit/components/navigation.test.jsx @@ -128,14 +128,49 @@ describe('Navigation', () => { childContextTypes: {store} }); - intlWrapper.children().find('Navigation') + const navInstance = intlWrapper.children().find('Navigation') .instance(); const twoMin = 2 * 60 * 1000; expect(setTimeout).toHaveBeenLastCalledWith(expect.any(Function), twoMin); + expect(navInstance.messageCountTimeoutId).not.toEqual(-1); // Advance timers passed the intial two minutes. jest.advanceTimersByTime(twoMin + 1); expect(setTimeout).toHaveBeenLastCalledWith(expect.any(Function), twoMin * 2); expect(props.getMessageCount).toHaveBeenCalled(); + expect(navInstance.messageCountTimeoutId).not.toEqual(-1); + }); + test('Component cancels timers when it unmounts', () => { + store = mockStore({ + navigation: { + registrationOpen: false + }, + messageCount: { + messageCount: 5 + } + }); + const props = { + user: { + thumbnailUrl: 'scratch.mit.edu', + username: 'auser' + }, + getMessageCount: jest.fn() + }; + const intlWrapper = mountWithIntl( + , {context: {store}, + childContextTypes: {store} + }); + + const navInstance = intlWrapper.children().find('Navigation') + .instance(); + const twoMin = 2 * 60 * 1000; + expect(setTimeout).toHaveBeenLastCalledWith(expect.any(Function), twoMin); + expect(navInstance.messageCountTimeoutId).not.toEqual(-1); + debugger; + navInstance.componentWillUnmount(); + expect(clearTimeout).toHaveBeenCalledWith(expect.any(Number)); + expect(navInstance.messageCountTimeoutId).toEqual(-1); }); test('pollForMessages polls for messages 5 times', () => { @@ -170,8 +205,8 @@ describe('Navigation', () => { let twoMinInMs = 2 * 60 * 1000; // 2 minutes in ms. navInstance.pollForMessages(twoMinInMs); + expect(navInstance.messageCountTimeoutId).not.toEqual(-1); // Check that we set the timeout to backoff exponentially - for (let count = 1; count < 5; ++count) { jest.advanceTimersByTime(twoMinInMs + 1); expect(setTimeout).toHaveBeenLastCalledWith(expect.any(Function), twoMinInMs * 2); @@ -187,5 +222,6 @@ describe('Navigation', () => { // setTimeout happens 1 fewer since the original call to // pollForMessages isn't counted here. expect(setTimeout).toHaveBeenCalledTimes(4); + expect(navInstance.messageCountTimeoutId).not.toEqual(-1); }); }); From a67a34a06c9fd99d21a041cf74278b609bb3f49a Mon Sep 17 00:00:00 2001 From: picklesrus Date: Thu, 30 Jul 2020 11:08:34 -0400 Subject: [PATCH 5/7] remove stray debugger --- test/unit/components/navigation.test.jsx | 1 - 1 file changed, 1 deletion(-) diff --git a/test/unit/components/navigation.test.jsx b/test/unit/components/navigation.test.jsx index fa180f63a..924e83551 100644 --- a/test/unit/components/navigation.test.jsx +++ b/test/unit/components/navigation.test.jsx @@ -167,7 +167,6 @@ describe('Navigation', () => { const twoMin = 2 * 60 * 1000; expect(setTimeout).toHaveBeenLastCalledWith(expect.any(Function), twoMin); expect(navInstance.messageCountTimeoutId).not.toEqual(-1); - debugger; navInstance.componentWillUnmount(); expect(clearTimeout).toHaveBeenCalledWith(expect.any(Number)); expect(navInstance.messageCountTimeoutId).toEqual(-1); From 25b9bacd0e650e1c257960627439c1b8809977a6 Mon Sep 17 00:00:00 2001 From: picklesrus Date: Thu, 6 Aug 2020 13:13:12 -0400 Subject: [PATCH 6/7] Update comment to match code --- src/components/navigation/www/navigation.jsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/navigation/www/navigation.jsx b/src/components/navigation/www/navigation.jsx index 81c83681e..7091ce434 100644 --- a/src/components/navigation/www/navigation.jsx +++ b/src/components/navigation/www/navigation.jsx @@ -69,7 +69,7 @@ class Navigation extends React.Component { pollForMessages (ms) { this.props.getMessageCount(this.props.user.username); - // We only poll if it has been less than 30 minutes. + // We only poll if it has been less than 32 minutes. // Chances of someone actively using the page for that long without // a navigation is low. if (ms < 32 * 60 * 1000) { // 32 minutes From 7d470131d7f2784fe33ffa5dcdd020849acd5697 Mon Sep 17 00:00:00 2001 From: picklesrus Date: Thu, 6 Aug 2020 13:22:49 -0400 Subject: [PATCH 7/7] Add check for existence of messageTimeoutId before clearing the timeout. --- src/components/navigation/www/navigation.jsx | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/components/navigation/www/navigation.jsx b/src/components/navigation/www/navigation.jsx index 7091ce434..39e69c242 100644 --- a/src/components/navigation/www/navigation.jsx +++ b/src/components/navigation/www/navigation.jsx @@ -47,8 +47,10 @@ class Navigation extends React.Component { const twoMinInMs = 2 * 60 * 1000; this.messageCountTimeoutId = setTimeout(this.pollForMessages.bind(this, twoMinInMs), twoMinInMs); } else { - // clear message count check, and set to default id. - clearTimeout(this.messageCountTimeoutId); + // Clear message count check, and set to default id. + if (this.messageCountTimeoutId !== -1) { + clearTimeout(this.messageCountTimeoutId); + } this.props.setMessageCount(0); this.messageCountTimeoutId = -1; }