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

feat: implement an async iterable request class #1644

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

chdh
Copy link
Collaborator

@chdh chdh commented Aug 1, 2024

This PR implements an async iterable request class.

The new class IterableRequest is implemented as a super class of the normal Request class. It is an AsyncIterable providing an AsyncIterator and can be used with for await.

Usage:

const request = new IterableRequest("select 42, 'hello world'");
connection.execSql(request);
for await (const item of request) {
  console.log(item.row);
}

A unit test is included. The API documentation has yet to be written.

Implements #1550

Copy link

codecov bot commented Aug 1, 2024

Codecov Report

Attention: Patch coverage is 83.69565% with 15 lines in your changes missing coverage. Please review.

Project coverage is 78.39%. Comparing base (65ab37d) to head (9691e11).
Report is 16 commits behind head on master.

Files with missing lines Patch % Lines
src/iterable-request.ts 83.69% 9 Missing and 6 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1644      +/-   ##
==========================================
- Coverage   79.01%   78.39%   -0.63%     
==========================================
  Files          93       91       -2     
  Lines        4876     4947      +71     
  Branches      937      948      +11     
==========================================
+ Hits         3853     3878      +25     
- Misses        720      768      +48     
+ Partials      303      301       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@arthurschreiber
Copy link
Collaborator

First of all, this is super exciting - thank you for working on this! ❤️

I haven't taken a super deep look into the implementation yet, but my idea for this was always to change the API for request execution to something like this:

// Create a new request, but without callback.
const request = new Request('select a, b, c from foobar');

// `using` so any resources are free'd to make the connection reusable in case some error happens in the user's code
using response = await connection.execSql(request);

// `.values` for a rows as arrays
// `.objects` for rows as objects
for await (const [a, b, c] of response.values()) {
  ...
}

Obviously, making such a profound change to the API is not simple, and there's going to be a lot of concerns around backwards compatibility and things like that.

Comment on lines +247 to +261
[Symbol.asyncIterator](): AsyncIterator<IterableRequestItem> {
return this.iterator;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you thought about implementing this through using events.on? I haven't tested this, but it would allow you to get rid of the custom iterator implementation.

  constructor(sqlTextOrProcedure: string | undefined, options?: IterableRequestOptions) {
    super(sqlTextOrProcedure, completionCallback, options);
    const controller = new AbortController();
    this.doneSignal = controller.signal;

    function completionCallback(error: Error | null | undefined) {
       controller.abort(error);
    }
  }

  [Symbol.asyncIterator](): AsyncIterator<IterableRequestItem> {
    return events.on(this, 'row', { signal: this.doneSignal });
  }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. I didn't know about events.on(). It does more or less the same as my controller and iterator classes.
I will refactor my code into using events.on().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since Tedious still has to support Node 18, I had a look at that old implementation of event.on().
The close event was not yet implemented in that version. I can't find a proper way to end the iteration so that the remaining items in the FIFO are still processed. abortListener() calls errorHandler() which calls toError.reject(err) which rejects the waiting Promise,
Calling iterator.return() resolves the waiting Promise with done = true.

Should I use my implementation for old Node versions and event.on() for new Node versions?

}

type RowData = ColumnValue[] | Record<string, ColumnValue>; // type variant depending on config.options.useColumnNames
type ColumnMetadataDef = ColumnMetadata[] | Record<string, ColumnMetadata>; // type variant depending on config.options.useColumnNames
Copy link
Collaborator

@arthurschreiber arthurschreiber Aug 1, 2024

Choose a reason for hiding this comment

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

config.options.useColumnNames is super bad API design, and I'd prefer if we could not propagate this nonsense. 😬

The reason why it's bad API design is that the config value changes the shape of the data that is being returned from tedious. Flipping this config basically from true to false or the other way around basically breaks all code that was written with the other value set.

Much better would be to have an API that allows the code to explicitly either use objects or use arrays.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree. But as you wrote before, this would be a profound change, beyond the scope of this PR.

@chdh chdh force-pushed the iterable-request branch from 411fb00 to 4af51d8 Compare August 5, 2024 02:46
this.resultSetNo = 0;
this.fifo = [];
const fifoSize = options?.iteratorFifoSize ?? 1024;
this.fifoPauseLevel = fifoSize;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this is similar to highWaterMark on regular streams? I.e. once the buffer (here called fifo) reaches that size, no more data will be buffered and the underlying request will be paused?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. Do you suggest to rename it to this.fifoHighWaterMark? And also rename the option parameter?

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

Successfully merging this pull request may close these issues.

2 participants