Skip to content

Commit

Permalink
Set retry limit for LastFMImporter (#1339)
Browse files Browse the repository at this point in the history 
* Set retry limit for LastFMImporter

* Export LASTFM_RETRIES and use in tests

* Document getPage and remove magic number 4

* Await on retry

* Rewrite getPage retry logic and fix associated tests

* Replace setTimeout function in LastFM getPage tests

Jest fake timers don't play well with promises and setTimeout so we replace the setTimeout function and don't have to wait for the real timeout.
See jestjs/jest#7151

* LastFMImporter: Retry only on 4xx errors

* LFM importer: throw error instead of returning null

Co-authored-by: Monkey Do <contact@monkeydo.digital>
  • Loading branch information
@amCap1712 @MonkeyDo
amCap1712 and MonkeyDo authored Mar 17, 2021
1 parent b3a5160 commit 987aa9a
Show file tree
Hide file tree
Showing 2 changed files with 157 additions and 54 deletions.
149 changes: 121 additions & 28 deletions listenbrainz/webserver/static/js/src/LastFMImporter.test.tsx
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as React from "react";
import { mount, shallow } from "enzyme";
import LastFmImporter from "./LastFMImporter";
import LastFmImporter, { LASTFM_RETRIES } from "./LastFMImporter";
// Mock data to test functions
import * as page from "./__mocks__/page.json";
import * as getInfo from "./__mocks__/getInfo.json";
Expand Down Expand Up @@ -133,6 +133,22 @@ describe("getTotalNumberOfScrobbles", () => {
});

describe("getPage", () => {
const originalTimeout = window.setTimeout;
beforeAll(() => {
// Ugly hack: Jest fake timers don't play well with promises and setTimeout
// so we replace the setTimeout function.
// see https://github.com/facebook/jest/issues/7151
// @ts-ignore
window.setTimeout = (fn: () => void, _timeout: number): number => {
fn();
return _timeout;
};
});

afterAll(() => {
window.setTimeout = originalTimeout;
});

beforeEach(() => {
const wrapper = shallow<LastFmImporter>(<LastFmImporter {...props} />);
instance = wrapper.instance();
Expand All @@ -147,22 +163,13 @@ describe("getPage", () => {
});

it("should call with the correct url", () => {
instance.getPage(1);
instance.getPage(1, LASTFM_RETRIES);

expect(window.fetch).toHaveBeenCalledWith(
`${props.lastfmApiUrl}?method=user.getrecenttracks&user=${instance.state.lastfmUsername}&api_key=${props.lastfmApiKey}&from=1&page=1&format=json`
);
});

it("should call encodeScrobbles", async () => {
// Mock function for encodeScrobbles
LastFmImporter.encodeScrobbles = jest.fn(() => ["foo", "bar"]);

const data = await instance.getPage(1);
expect(LastFmImporter.encodeScrobbles).toHaveBeenCalledTimes(1);
expect(data).toEqual(["foo", "bar"]);
});

it("should retry if 50x error is recieved", async () => {
// Mock function for fetch
window.fetch = jest.fn().mockImplementation(() => {
Expand All @@ -172,11 +179,51 @@ describe("getPage", () => {
});
});

await instance.getPage(1);
// There is no direct way to check if retry has been called
expect(setTimeout).toHaveBeenCalledTimes(1);
const getPageSpy = jest.spyOn(instance, "getPage");
let finalValue;
try {
finalValue = await instance.getPage(1, LASTFM_RETRIES);
} catch (err) {
expect(getPageSpy).toHaveBeenCalledTimes(1 + LASTFM_RETRIES);
expect(finalValue).toBeUndefined();

// This error message is also displayed to the user
expect(err).toEqual(
new Error(
`Failed to fetch page 1 from last.fm after ${LASTFM_RETRIES} retries.`
)
);
}
});

jest.runAllTimers();
it("should return the expected value if retry is successful", async () => {
// Mock function for fetch
window.fetch = jest
.fn()
.mockImplementationOnce(() => {
return Promise.resolve({
ok: false,
status: 503,
});
})
.mockImplementationOnce(() => {
return Promise.resolve({
ok: false,
status: 503,
});
})
.mockImplementationOnce(() => {
return Promise.resolve({
ok: true,
json: () => Promise.resolve(page),
});
});

const getPageSpy = jest.spyOn(instance, "getPage");
const finalValue = await instance.getPage(1, LASTFM_RETRIES);

expect(getPageSpy).toHaveBeenCalledTimes(3);
expect(finalValue).toEqual(encodeScrobbleOutput);
});

it("should skip the page if 40x is recieved", async () => {
Expand All @@ -187,24 +234,59 @@ describe("getPage", () => {
status: 404,
});
});
const getPageSpy = jest.spyOn(instance, "getPage");
const finalValue = await instance.getPage(1, LASTFM_RETRIES);

await instance.getPage(1);
expect(setTimeout).not.toHaveBeenCalled();
expect(getPageSpy).toHaveBeenCalledTimes(1);
expect(finalValue).toEqual(undefined);
});

it("should retry if there is any other error", async () => {
// Mock function for fetch
it("should skip the page if 30x is recieved", async () => {
// Mock function for failed fetch
window.fetch = jest.fn().mockImplementation(() => {
return Promise.resolve({
ok: true,
json: () => Promise.reject(),
ok: false,
status: 301,
});
});
const getPageSpy = jest.spyOn(instance, "getPage");
const finalValue = await instance.getPage(1, LASTFM_RETRIES);

await instance.getPage(1);
// There is no direct way to check if retry has been called
expect(setTimeout).toHaveBeenCalledTimes(1);
jest.runAllTimers();
expect(getPageSpy).toHaveBeenCalledTimes(1);
expect(finalValue).toEqual(undefined);
});

it("should retry if there is any other error", async () => {
// Mock function for fetch
window.fetch = jest
.fn()
.mockImplementationOnce(() => {
return Promise.resolve({
ok: true,
json: () => Promise.reject(new Error("Error")),
});
})
.mockImplementationOnce(() => {
return Promise.resolve({
ok: true,
json: () => Promise.resolve(page),
});
});

const getPageSpy = jest.spyOn(instance, "getPage");
const finalValue = await instance.getPage(1, LASTFM_RETRIES);

expect(getPageSpy).toHaveBeenCalledTimes(2);
expect(finalValue).toEqual(encodeScrobbleOutput);
});

it("should call encodeScrobbles", async () => {
// Mock function for encodeScrobbles
LastFmImporter.encodeScrobbles = jest.fn(() => ["foo", "bar"]);

const data = await instance.getPage(1, LASTFM_RETRIES);
expect(LastFmImporter.encodeScrobbles).toHaveBeenCalledTimes(1);
expect(data).toEqual(["foo", "bar"]);
});
});

Expand Down Expand Up @@ -331,8 +413,9 @@ describe("LastFmImporter Page", () => {
});

describe("importLoop", () => {
let wrapper: any;
beforeEach(() => {
const wrapper = shallow<LastFmImporter>(<LastFmImporter {...props} />);
wrapper = shallow<LastFmImporter>(<LastFmImporter {...props} />);
instance = wrapper.instance();
instance.setState({ lastfmUsername: "dummyUser" });
// needed for startImport
Expand Down Expand Up @@ -381,7 +464,7 @@ describe("importLoop", () => {
});

it("should show error message on unhandled exception / network error", async () => {
const errorMsg = "Testing: something went wrong !!!";
const errorMsg = `Some error`;
// Mock function for failed importLoop
instance.importLoop = jest.fn().mockImplementation(async () => {
const error = new Error();
Expand All @@ -390,9 +473,19 @@ describe("importLoop", () => {
throw error;
});

const consoleErrorSpy = jest.spyOn(console, "error");

// startImport shouldn't throw error
await expect(instance.startImport()).resolves.toBe(null);
// verify message is failure message
expect(instance.state.msg?.props.children).toContain(errorMsg);
expect(instance.state.msg?.props.children).toContain(
" We were unable to import from LastFM, please try again."
);
expect(instance.state.msg?.props.children).toContain(
"If the problem persists please contact us."
);
expect(instance.state.msg?.props.children).toContain("Error: Some error");

expect(consoleErrorSpy).toHaveBeenCalledWith(new Error("Some error"));
});
});
62 changes: 36 additions & 26 deletions listenbrainz/webserver/static/js/src/LastFMImporter.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@ import { IconProp } from "@fortawesome/fontawesome-svg-core";
import APIService from "./APIService";
import Scrobble from "./Scrobble";

import LastFMImporterModal from "./LastFMImporterModal";

export const LASTFM_RETRIES = 3;
export type ImporterProps = {
user: {
id?: string;
Expand Down Expand Up @@ -151,19 +150,19 @@ export default class LastFmImporter extends React.Component<
}
}

async getPage(page: number) {
/*
* Fetch page from Last.fm
*/
/*
* @param {number} page - the page to fetch from Last.fm
* @param {number} retries - number times to retry in case of errors other than 40x
* Fetch page from Last.fm
* @returns Returns an array of Listens if successful, null if it exceeds the max number of retries (consider that an error)
* and undefined if we receive a 40X error for the page
*/
async getPage(
page: number,
retries: number
): Promise<Array<Listen> | undefined> {
const { lastfmUsername } = this.state;

const retry = (reason: string) => {
// eslint-disable-next-line no-console
console.warn(
`${reason} while fetching last.fm page=${page}, retrying in 3s`
);
setTimeout(() => this.getPage(page), 3000);
};
const timeout = 3000;

const url = `${
this.lastfmURL
Expand All @@ -189,17 +188,26 @@ export default class LastFmImporter extends React.Component<
this.countReceived += payload.length;
return payload;
}
// Retry if we receive a 5xx server error
if (/^5/.test(response.status.toString())) {
retry(`Got ${response.status}`);
} else {
// ignore 40x
// console.warn(`Got ${response.status} while fetching page last.fm page=${page}, skipping`);
throw new Error(`Status ${response.status}`);
}
} catch {
} catch (err) {
// Retry if there is a network error
retry("Network error");
// eslint-disable-next-line no-console
console.warn(`Error while fetching last.fm page ${page}:`, err);
if (retries <= 0) {
throw new Error(
`Failed to fetch page ${page} from last.fm after ${LASTFM_RETRIES} retries.`
);
}
// eslint-disable-next-line no-console
console.warn(`Retrying in ${timeout / 1000}s, ${retries} retries left`);
await new Promise((resolve) => setTimeout(resolve, timeout));
// eslint-disable-next-line no-return-await
return await this.getPage(page, retries - 1);
}
return null;
return undefined;
}

static getlastImportedString(listen: Listen) {
Expand Down Expand Up @@ -276,7 +284,8 @@ export default class LastFmImporter extends React.Component<
while (this.page > 0) {
// Fixing no-await-in-loop will require significant changes to the code, ignoring for now
this.lastImportedString = "...";
const payload = await this.getPage(this.page); // eslint-disable-line
const payload = await this.getPage(this.page, LASTFM_RETRIES); // eslint-disable-line

if (payload) {
// Submit only if response is valid
this.submitPage(payload);
Expand Down Expand Up @@ -332,11 +341,12 @@ export default class LastFmImporter extends React.Component<
// import failed, show final message on unhandled exception / unrecoverable network error
finalMsg = (
<p>
<FontAwesomeIcon icon={faTimes as IconProp} /> Import failed due to a
network error, please retry.
<FontAwesomeIcon icon={faTimes as IconProp} /> We were unable to
import from LastFM, please try again.
<br />
Message: &quot;{err.message}&quot;
If the problem persists please contact us.
<br />
{err.toString()}
<br />
<span style={{ fontSize: `${10}pt` }}>
<a href={`${profileUrl}`}>
Expand All @@ -346,7 +356,7 @@ export default class LastFmImporter extends React.Component<
</p>
);
// eslint-disable-next-line no-console
console.debug(err);
console.error(err);
this.setState({ canClose: true, msg: finalMsg });
return Promise.resolve(null);
}
Expand Down

0 comments on commit 987aa9a

Please sign in to comment.