[script-compiler] Mapping sent and received messages and avoiding race conditions

[otariidae]

I'd like to have a one-to-one correspondence between the text I send to the Textlint worker and the lint result I receive from the Textlint worker.
However, the order I received results from the Textlint worker can differ from the one I posted texts to the worker probably because kernel.lintText is async. Ref:

editor/packages/@textlint/script-compiler/src/CodeGenerator/worker-codegen.ts

Lines 125 to 136 in b32d016

	return kernel.lintText(data.text, {
	rules: rules,
	filterRules: config.filterRules,
	plugins: config.plugins,
	filePath: "/path/to/README" + data.ext,
	ext: data.ext,
	}).then(result => {
	return self.postMessage({
	command: "lint:result",
	result
	});
	});

So relying on message reception order can cause a race condition. Pseudocode:

worker.addEventListener("message", (event) => {
  // lint result of text#2 comes first
  // lint result of text#1 comes next
})
worker.postMessage({
  command: "lint",
  text: "text#1 very long text that takes a long time to lint",
  ext: ".txt"
})
worker.postMessage({
  command: "lint",
  text: "text#2 short text; lint finishes quickly",
  ext: ".txt"
})

More higher-level example:

editor/packages/textchecker-element/index.ts

Lines 41 to 63 in b32d016

	const lintText: LintEngineAPI["lintText"] = async ({ text }: { text: string }): Promise<TextlintResult[]> => {
	updateStatus("linting...");
	return new Promise((resolve, _reject) => {
	worker.addEventListener(
	"message",
	function (event) {
	const data: TextlintWorkerCommandResponse = event.data;
	if (data.command === "lint:result") {
	resolve([data.result]);
	}
	updateStatus("linted");
	},
	{
	once: true
	}
	);
	return worker.postMessage({
	command: "lint",
	text,
	ext: ext
	} as TextlintWorkerCommandLint);
	});
	};

const texts = ["text#1 very long text that takes a long time to lint", "text#2 short text; lint finishes quickly"]
const results = await Promise.all(texts => lintText({ text }))
// [<lint result of text#2>, <lint result of text#2>]
// lint result of text#1 goes away

Some Web Worker libraries use IDs in messages. I think it can be one of the ideas to solve this problem.
For example, comlink:

https://github.com/GoogleChromeLabs/comlink/blob/dffe9050f63b1b39f30213adeb1dd4b9ed7d2594/src/comlink.ts#L596-L615

For another example, minlink:

https://github.com/mizchi/minlink/blob/98f0eed1b1fc00a51709e07c6fe3e18232cdfaad/src/shared.ts#L52-L61

[azu]

I'm ok that adding identifier to messages.

Specifically, the worker code receives the id and puts the id against the xxx:result as follows?

const id = crypto.randomUUID();
worker.addEventListener("message", (event) => {
  if(event.data.id === id) {
    console.log("result of #1");
  }
})
worker.postMessage({
  id, // <= added
  command: "lint",
  text: "text#1 short text; lint finishes quickly",
  ext: ".txt"
})

[otariidae]

Yes. The way you showed looks good to me.

[azu]

Another Discussion Points

• Is the naming correct?
  • metadata property is another option, but id is simpler.
  • metdata: {} or id: string
• Should id is required property not to be optional?
  • I prefer to make it required property

[otariidae]

• I prefer id as it looks like a common practice in other libraries such as comlink and typed-worker
• I also prefer to require id property to avoid the pitfall of race condition by design

[azu]

I agree with you.

TODO

• add id to messaging as required property
• fix usage https://github.com/search?q=repo%3Atextlint%2Feditor%20postMessage&type=code

Welcome to PR

[otariidae]

I am trying to make a PR and found another discussion point:

• Error handling
  • Textlint worker should notify when id is missing or something goes wrong
  • Throwing Error in the worker can be handled by worker.addEventListener("error", ...) but Error object does not have id
    • This cause a race condition too
  • How should the worker notify error information with id?

One of my ideas is that the worker responds with a newly-defined format like TextlintWorkerCommandResponseError instead of throwing Error.

[azu]

Yeah, error response is reasonable.

[otariidae]

Sorry, I found a simple oversight in my idea. When id is missing, the worker cannot respond with id.
My idea works only if id exists and something goes wrong, for example, when kernel.lintText fails.
I am considering another way or someone have good idea?

[otariidae]

My another idea is just to throw Error if id is missing to show that the worker will never work without id, and to respond with TextlintWorkerCommandResponseError if id exists and Textlint fails.

[azu]

I think that worker should just post error message when id is missing.
It is global error like Window: error event.
If the command has id and some error on liting, worker post error with id like { message: "xxx", id/commandId: "xxx" }

User always listen the error event.

worker.addEventListener("message", (event) => {
    const data: TextlintWorkerCommandResponse = event.data;
    if (data.command === "error") {
       console.error(data.error); // missing id error
    }
});
worker.postMessage({
  command: "lint",
  text: "text#1 short text; lint finishes quickly",
  ext: ".txt"
});

const id = crypto.randomUUID();
worker.addEventListener("message", (event) => {
    const data: TextlintWorkerCommandResponse = event.data;
    if (data.command === "error") {
        console.error(data.error, data.error.id); // something error at `id`.
    }
});
worker.postMessage({
  id, 
  command: "lint",
  text: "text#1 short text; lint finishes quickly",
  ext: ".txt"
});

[azu]

It may be better to proceed separately with the PR to add the id and the PR to make the id mandatory.
The latter is a breaking change, so there is a lot to think about.

[otariidae]

My idea that just throws Error when missing id looks like this:

const id = crypto.randomUUID();
worker.addEventListener("message", function onResult(event) {
    const data: TextlintWorkerCommandResponse = event.data;
    if (data.id === id) {
        if (data.error) {
            console.error(data.error); // id exists but something went wrong
        } else {
            console.log(data.result) // succeeded
        }
        worker.removeEventListener("message", onResult);
        worker.removeEventListener("error", onError);
    }
});
worker.addEventListener("error", function onError(error) {
    console.error(error); // missing id error
    worker.removeEventListener("message", onResult);
    worker.removeEventListener("error", onError);
});

Or more comprehensive idea is that script-compiler provides client-side code to force Textlint message protocol as some other libraries do. Though this is complicated.

const textlint = wrap(new Worker("textlint-worker.js"));
const results = await Promise.all([
    textlint.lint("text#1 short text; lint finishes quickly", ".txt"),
    textlint.lint("text#2 short text; lint finishes quickly", ".txt")
]):
// safe for race conditions by default
// provides best way to work with Textlint worker, id and error handling included

[azu]

Ah, I did not know that catch the exception on the worker via worker.addEventListener("error", ....
It is better than command === "error".

[otariidae]

I am considering that it may be better to first create a PR that provides error response messages for error handling, then make a PR that makes id required.