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

Add experimental support of AbortSignal #55

Merged
merged 3 commits into from
Nov 15, 2022
Merged

Add experimental support of AbortSignal #55

merged 3 commits into from
Nov 15, 2022

Conversation

vweevers
Copy link
Member

In #50 I made an early start with this, but I made two mistakes:

  1. Putting the signal in per-method options, e.g. iterator.next({ signal }), rather than in constructor options, i.e. db.iterator({ signal }). The former would not be accessible in a for await...of and come with a performance penalty.
  2. Thinking it could replace abortOnClose (Add hidden abortOnClose option to iterators #21) which is a separate issue. I'll leave it at that, because it only matters for many-level and can be solved and explained later.

Anyway, this PR enables the following:

const abortController = new AbortController()
const signal = abortController.signal

// Will result in 'aborted' log
abortController.abort()

try {
  for await (const entry of db.iterator({ signal })) {
    console.log(entry)
  }
} catch (err) {
  if (err.code === 'LEVEL_ABORTED') {
    console.log('aborted')
  }
}

Further explained in the README, for the public API and the private API (second paragraph).

This PR also fixes small bugs in the open() and close() methods of multiple classes, as follow-up for #50.

@vweevers vweevers added the enhancement New feature or request label Nov 14, 2022
@vweevers vweevers added this to the 2.0.0 milestone Nov 14, 2022
Comment on lines +13 to +23
// TODO: we should set name to AbortError for web compatibility. See:
// https://dom.spec.whatwg.org/#aborting-ongoing-activities
// https://github.com/nodejs/node/pull/35911#discussion_r515779306
//
// But I'm not sure we can do the same for errors created by a Node-API addon (like
// classic-level) so for now this behavior is undocumented and folks should use the
// LEVEL_ABORTED code instead.
get name () {
return 'AbortError'
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI

README.md Outdated Show resolved Hide resolved
abstract-level.js Outdated Show resolved Hide resolved
@vweevers vweevers merged commit a7d688b into v2 Nov 15, 2022
@vweevers vweevers deleted the signals branch November 15, 2022 21:58
vweevers added a commit that referenced this pull request Nov 19, 2022
vweevers added a commit that referenced this pull request Jan 27, 2024
vweevers added a commit that referenced this pull request Jan 27, 2024
And require error `name` to be `AbortError`, as follow-up for
#55 (comment).
vweevers added a commit that referenced this pull request Jan 27, 2024
vweevers added a commit that referenced this pull request Jan 27, 2024
And require error `name` to be `AbortError`, as follow-up for
#55 (comment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

1 participant