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.

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: remove Promise.race causing race condition #4

Merged
merged 2 commits into from
Apr 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
22 changes: 21 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,27 @@ To use from JSR, install the
package and then import into a module:

```js
import { parallelizeGeneratorPromises } from "parallelize-generator-promises";
import { parallelizeGeneratorPromises } from "@reda/parallelize-generator-promises";
```

### Polyfilling Promise.withResolvers

If you are using Node.js v20 or below, you will require a polyfill for
[`Promise.withResolvers`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/withResolvers).

A polyfill for this is available in a separate export as part of this package.
To use the provided polyfill, import it at the entrypoint of your application.

npm:

```js
import "parallelize-generator-promises/promise-with-resolvers-polyfill";
```

jsr:

```js
import "@reda/parallelize-generator-promises/promise-with-resolvers-polyfill";
```

## Usage
Expand Down
5 changes: 4 additions & 1 deletion deno.jsonc
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
{
"name": "@reda/parallelize-generator-promises",
"version": "1.1.0",
"exports": "./mod.ts",
"exports": {
".": "./mod.ts",
"./promise-with-resolvers-polyfill": "./promise-with-resolvers-polyfill.ts"
},
"exclude": ["CHANGELOG.md", "npm"],
"imports": {
"@deno/dnt": "jsr:@deno/dnt@^0.41.0",
Expand Down
5 changes: 4 additions & 1 deletion deno.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion mod.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ it("should not buffer more promises once maxBufferedPromises is reached", async
});
for await (
const result of parallelizeGeneratorPromises(function* () {
yield [Promise.resolve(1)];
yield [promise.then(() => 1)];
yield [promise.then(() => 2)]; // generator should wait here until promise is resolved
yield [promise.then(() => 3)];
generatorYielded = true;
Expand Down
46 changes: 29 additions & 17 deletions mod.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,32 +61,42 @@ export async function* parallelizeGeneratorPromises<T>(
{ maxBufferedPromises }: ParallelizeGeneratorPromisesOptions = {},
): AsyncGenerator<T, void, undefined> {
const bufferedPromises: Promise<T>[] = [];
const resolvers: ((promise: Promise<T>) => void)[] = [];
const bufferedPromisesResolvers: ((promise: Promise<T>) => void)[] = [];
let done = false;
let error;
let promiseResolve: () => void;
let promise = new Promise<void>((resolve) => (promiseResolve = resolve));
let {
promise: inputGeneratorYieldPromise,
resolve: inputGeneratorYieldPromiseResolve,
} = Promise["withResolvers"]<void>();
let {
promise: outputGeneratorYieldPromise,
resolve: outputGeneratorYieldPromiseResolve,
} = Promise["withResolvers"]<void>();

(async () => {
try {
for await (const promises of generator()) {
while (
maxBufferedPromises && bufferedPromises.length >= maxBufferedPromises
) {
await Promise.race(bufferedPromises).catch(() => {});
await outputGeneratorYieldPromise;
}
for (const bufferedPromise of promises) {
bufferedPromises.push(
new Promise((resolve) => resolvers.push(resolve)),
);
bufferedPromise
for (const promise of promises) {
const { promise: bufferedPromise, resolve: bufferedPromiseResolve } =
Promise["withResolvers"]<T>();
bufferedPromises.push(bufferedPromise);
bufferedPromisesResolvers.push(bufferedPromiseResolve);
promise
.catch(() => {})
.finally(() => {
resolvers.shift()!(bufferedPromise);
bufferedPromisesResolvers.shift()!(promise);
});
}
promiseResolve!();
promise = new Promise<void>((resolve) => (promiseResolve = resolve));
inputGeneratorYieldPromiseResolve();
({
promise: inputGeneratorYieldPromise,
resolve: inputGeneratorYieldPromiseResolve,
} = Promise["withResolvers"]<void>());
}
} catch (e) {
error = e;
Expand All @@ -96,12 +106,14 @@ export async function* parallelizeGeneratorPromises<T>(
})();

while (!done) {
await promise;
await inputGeneratorYieldPromise;
while (bufferedPromises.length > 0) {
const bufferedPromise = bufferedPromises.shift();
if (bufferedPromise) {
yield bufferedPromise;
}
yield bufferedPromises.shift()!;
outputGeneratorYieldPromiseResolve();
({
promise: outputGeneratorYieldPromise,
resolve: outputGeneratorYieldPromiseResolve,
} = Promise["withResolvers"]<void>());
}
}

Expand Down
45 changes: 45 additions & 0 deletions promise-with-resolvers-polyfill.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import { assertEquals, assertInstanceOf, assertRejects } from "@std/assert";
import { it } from "@std/testing/bdd";
import { assertSpyCallArgs, assertSpyCalls, spy } from "@std/testing/mock";

// @ts-expect-error: testing polyfill
delete Promise.withResolvers;
await import("./promise-with-resolvers-polyfill.ts");

it("should allow for custom promises", () => {
const resolveSpy = spy();
const rejectSpy = spy();
class NotPromise<T> {
constructor(
executor: (
resolve: (value: T) => void,
reject: (reason: unknown) => void,
) => void,
) {
executor(resolveSpy, rejectSpy);
}
}

const { promise, resolve, reject } = Promise.withResolvers.call(NotPromise);
assertInstanceOf(promise, NotPromise);

resolve("resolved");
assertSpyCalls(resolveSpy, 1);
assertSpyCallArgs(resolveSpy, 0, ["resolved"]);

reject(new Error("rejected"));
assertSpyCalls(rejectSpy, 1);
assertSpyCallArgs(rejectSpy, 0, [new Error("rejected")]);
});

it("should resolve the given promise when calling resolve", async () => {
const { promise, resolve } = Promise.withResolvers();
resolve(1);
assertEquals(await promise, 1);
});

it("should reject the given promise when calling reject", async () => {
const { promise, reject } = Promise.withResolvers();
reject(new Error("test failure"));
await assertRejects(() => promise, Error, "test failure");
});
11 changes: 11 additions & 0 deletions promise-with-resolvers-polyfill.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
if (!Promise.withResolvers) {
Promise.withResolvers = function withResolvers<T>() {
let promiseResolve: PromiseWithResolvers<T>["resolve"];
let promiseReject: PromiseWithResolvers<T>["reject"];
const promise = new this<T>((resolve, reject) => {
promiseResolve = resolve;
promiseReject = reject;
});
return { resolve: promiseResolve!, reject: promiseReject!, promise };
};
}
1 change: 1 addition & 0 deletions scripts/prepare_release.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ await build({
esModule: false,
skipNpmInstall: true,
test: false,
typeCheck: false,
outDir: "./npm",
shims: {},
package: {
Expand Down