Skip to content

Add experimental support of AbortSignal#55

Merged
vweevers merged 3 commits into
v2from
signals
Nov 15, 2022
Merged

Add experimental support of AbortSignal#55
vweevers merged 3 commits into
v2from
signals

Conversation

@vweevers
Copy link
Copy Markdown
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 thread lib/errors.js
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
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

FYI

Comment thread README.md Outdated
Comment thread abstract-level.js Outdated
@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

Archived in project

Development

Successfully merging this pull request may close these issues.

1 participant