Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(persister): Handle concurrent find requests #88

Merged
merged 1 commit into from
Aug 8, 2018
Merged

fix(persister): Handle concurrent find requests #88

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 23 additions & 14 deletions packages/@pollyjs/persister/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ const CREATOR_NAME = 'Polly.JS';
export default class Persister {
constructor(polly) {
this.polly = polly;
this.cache = new Map();
this.pending = new Map();
this._cache = new Map();
}

static get type() {
Expand Down Expand Up @@ -116,31 +116,40 @@ export default class Persister {
}

async find(recordingId) {
if (this.cache.has(recordingId)) {
return this.cache.get(recordingId);
}
const { _cache: cache } = this;

if (!cache.has(recordingId)) {
const findRecording = async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth naming this anonymous fn for a better callstack

const recording = await this.findRecording(recordingId);

if (recording) {
this.assert(
`Recording with id '${recordingId}' is invalid. Please delete the recording so a new one can be created.`,
recording.log && recording.log.creator.name === CREATOR_NAME
);

return recording;
} else {
cache.delete(recordingId);

const recording = await this.findRecording(recordingId);
return null;
}
};

if (recording) {
this.assert(
`Recording with id '${recordingId}' is invalid. Please delete the recording so a new one can be created.`,
recording.log && recording.log.creator.name === CREATOR_NAME
);
this.cache.set(recordingId, recording);
cache.set(recordingId, findRecording());
}

return recording;
return cache.get(recordingId);
}

async save(recordingId) {
await this.saveRecording(...arguments);
this.cache.delete(recordingId);
this._cache.delete(recordingId);
}

async delete(recordingId) {
await this.deleteRecording(...arguments);
this.cache.delete(recordingId);
this._cache.delete(recordingId);
}

async findEntry(pollyRequest) {
Expand Down
26 changes: 22 additions & 4 deletions packages/@pollyjs/persister/tests/unit/persister-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import Persister from '../../src';
import { timeout } from '@pollyjs/utils';

describe('Unit | Persister', function() {
it('should exist', function() {
Expand All @@ -19,24 +20,37 @@ describe('Unit | Persister', function() {
};

class CustomPersister extends Persister {
findRecording() {
async findRecording() {
callCounts.find++;
await timeout(1);

return recording;
}

saveRecording() {
async saveRecording() {
callCounts.save++;
await timeout(1);
}

deleteRecording() {
async deleteRecording() {
callCounts.delete++;
await timeout(1);
}
}

this.persister = new CustomPersister({});
});

it('should handle concurrent find requests', async function() {
await Promise.all([
this.persister.find('test'),
this.persister.find('test'),
Copy link
Contributor

@jasonmit jasonmit Aug 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we update the test to assert when adding another recordingId that it will increment?

i.e.:

this.persister.find('foo');
this.persister.find('foo');
this.persister.find('bar ');
/* count 2 */

Can you also add tests where a mutation, like delete, is done in between finds. Feels like a possible race condition. After this change, the second find would return the result of the first - correct? Which would not be valid. Since it was really deleted. In either case, we should add tests around this and patch any bugs. . You already added tests in the file previously that covers this :)

this.persister.find('test')
]);

expect(callCounts.find).to.equal(1);
});

it('caches', async function() {
await this.persister.find('test');
await this.persister.find('test');
Expand All @@ -49,7 +63,11 @@ describe('Unit | Persister', function() {
recording = null;

await this.persister.find('test');
await this.persister.find('test');
await Promise.all([
this.persister.find('test'),
this.persister.find('test'),
this.persister.find('test')
]);
await this.persister.find('test');

expect(callCounts.find).to.equal(3);
Expand Down