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

StreamablePromise is not following the Promise specification #122

Open
HCanber opened this issue Oct 29, 2024 · 1 comment
Open

StreamablePromise is not following the Promise specification #122

HCanber opened this issue Oct 29, 2024 · 1 comment

Comments

@HCanber
Copy link

HCanber commented Oct 29, 2024

This applies to classes extending StreamablePromise: StreamableRowPromise, StreamableReplicasPromise, StreamableScanPromise as of commit 2317bb6

StreamablePromise<T> is said to implement Promise<T> but is not following the specifications for promises. MDN describes what StreamablePromise needs to support to be a promise:

The eventual state of a pending promise can either be fulfilled with a value or rejected with a reason (error).
...
If the promise has already been fulfilled or rejected when a corresponding handler is attached, the handler will be called, so there is no race condition between an asynchronous operation completing and its handlers being attached.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise#description

So a promise doesn’t have to be awaited immediately. However, if the returned "promise" from for example cluster.searchQuery isn't awaited immediately – either using await or calling then()/catch()/finally() – the behavior and result are undefined.

The handler might get correct result back after being attached, if it's done before any data has been emitted to the internal EventEmitter, but if searchQuery has already received some data, the handler will miss som or all data, or in some cases the process might fail (I've seen process exit code 13 and the message Warning: Detected unsettled top-level await at file://....) .

This is because EventEmitter handlers are attached only when then()/catch()/finally() are called on
StreamablePromise<T>, which is too late.

Reasons for not attaching a handler immediately

If you expect a query to take some time, you might want to start it, do some other stuff, and then wait for the result.

const queryPromise = cluster.searchQuery(...)
await performOtherTask()
const result = await queryPromise

Workaround

The workaround is to make sure that a handler is always attached, for example by using .then(r => r). This will ensure that the internal EventEmitter handlers are hooked up in time.

const queryPromise = cluster.searchQuery(...).then(r => r)
await performOtherTask()
const result = await queryPromise

Fixing

I realize that this might be hard to fix in a reasonable way, but I'm submitting this for others to see.

For a future major version, one idea is that you separate EventEmitter/streaming functionality from Promise functionality so that the returned object doesn't handle both cases simultaneously.

For example:

const queryEmitter: EventEmitter = cluster.searchQuery(...)         // This is not a promise
const queryPromise: Promise = cluster.searchQuery(...).asPromise()  // This is not an EventEmitter

or by supplying different functions

const queryEmitter: EventEmitter = cluster.searchQueryAsStream(...)
const queryPromise: Promise = cluster.searchQuery(...)

In the meantime you could also expose the promise property so that instead of the .then(r =>r) workaround we could use .promise

const queryPromise = cluster.searchQuery(...).promise
await performOtherTask()
const result = await queryPromise
@thejcfactor
Copy link
Contributor

thejcfactor commented Nov 13, 2024

Hi @HCanber - I have created a ticket in our JIRA system (JSCBC-1301) for tracking the changes to address this issue. We are actively looking into what changes we might make, but due to the nature of StreamablePromise API, I don't think the changes will surface in the SDK until the next dot-minor release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants