Skip to content

Add signal option to Node.js API for cancellation support #9209

@adalinesimonian

Description

@adalinesimonian

What is the problem you're trying to solve?

When Stylelint is used as a language server, or any other long-running Node.js API consumer, there's no way to cancel a stylelint.lint() call that's already in progress. This means that if a user edits a document while a lint is running, the old lint continues running to completion even though its results are going to be thrown away.

In stylelint/vscode-stylelint#914, I'm adding an AbortController-based cancellation system to the extension's worker processes. When a document is re-edited, or a config change triggers re-validation, or so on, the in-flight worker request is aborted immediately so the worker can start handling the new request sooner. But the cancellation can only take effect around the stylelint.lint() call, that is, I can check before and after calling it, but can't interrupt anything that's happening inside lint() itself. The worker still has to wait for lint() to return before it can move on.

This means the entire rule execution phase runs even when we already know the result is stale. For multi-file linting via files, it's even worse given the volume of work with multiple files involved.

What solution would you like to see?

An optional signal property on LinterOptions, accepting a standard AbortSignal:

interface LinterOptions {
  // ...
  signal?: AbortSignal;
}

Using it, either from the language server or any other consumer, would look like this:

const controller = new AbortController();

// If the document changes before this resolves, call controller.abort() to cancel the lint.
const result = await stylelint.lint({
  code: document.getText(),
  codeFilename: filePath,
  signal: controller.signal,
});

There are a few good places to check signal.aborted that I can think of.

Location Granularity Notes
standalone.mjs, before each file in the files path Per-file Check before calling lintSource() inside the .map() callback
lintSource.mjs, before config resolution, before parsing, before rule execution, after rule execution Per-phase
lintPostcssResult.mjs, after each rule settles Per-rule(ish) Only catches aborts between async rules

The per-phase checks are straightforward, we just add if (signal?.aborted) throw signal.reason; guards at the boundaries of each phase of execution.

The per-rule checkpoint is more interesting because of how lintPostcssResult works. The rule execution loop synchronously enqueues all rules into performRules and then runs them with Promise.all(performRules). Each rule's promise could be wrapped with a post-completion signal check:

const performRules = ruleEntries.map(([ruleName, ruleFunction]) => {
  return Promise.all(postcssRoots.map(runRule)).then((result) => {
    if (signal?.aborted) {
      throw signal.reason;
    }

    return result;
  });
});

return Promise.all(performRules);

For synchronous rules, all the .then() callbacks drain in one microtask batch after all rules run, so the signal check is effectively the same as a single check after Promise.all. For async rules, there's now get a cancellation checkpoint after each rule finishes running, which is the best I can think to do without making rule execution serial.

Potentially, we could allow rules to opt in to more fine-grained cancellation by accepting the signal as an argument and checking it at their leisure. However, I think this is a larger change that is out of scope for what I'm proposing here, and potentially may be too much granularity for most use cases.

Some notes

When fix is used with files, fixed output is written to disk per-file as each one completes. If the signal is aborted mid-run, some files would already be written with fixes applied while the rest would still have their original content. However, this is the same behavior you'd get today by interrupting the CLI with Ctrl+C, so we're not really introducing any fundamentally new risk here. That said, it's probably worth mentioning in the API docs that aborting a run with fix set may leave files in a partially-fixed state. In any case, for the primary use case motivating this feature, there are are rarely if ever any disk writes happening, so it's a non-issue.

This change would be minimal, just requiring a small handful of if statements sprinkled in some parts of our codebase, alongside the .then() wrapper for rule execution promises. We don't need to restructure any control flow or fundamentally change how anything works, nor do we introduce any breaking changes. We're also not trailblazing any new patterns here, AbortController is widely used across Node.js core APIs and the broader ecosystem for exactly this kind of use case. So overall, this feels like an easy win with very little to practically no maintenance burden attached to it.

I'd be happy to put together a PR for this if there's interest. Certainly coming from the language server side, I'm very interested in having this feature here in Stylelint.

Metadata

Metadata

Labels

status: wipis being worked on by someone
No fields configured for Enhancement.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions