Add abort support [WIP] #2184
Add abort support [WIP] #2184
Conversation
|
@tomaszkiewicz is attempting to deploy a commit to the Gloo Team on Vercel. A member of the Team first needs to authorize it. |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 224193d in 2 minutes and 21 seconds. Click for details.
- Reviewed
1387lines of code in8files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. docs/abort-controller.md:207
- Draft comment:
The documentation is comprehensive. Consider mentioning that using{ once: true }for abort event listeners can help prevent memory leaks in long-lived streams. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. examples/typescript/abort-controller-example.ts:75
- Draft comment:
Typographical consistency: The comment for Example 3 uses 'Pre-emptive abort' while the function is named 'preemptiveAbortExample'. Consider using a consistent spelling (either with or without the hyphen) throughout the file. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment is technically correct about the inconsistency, this is an extremely minor stylistic issue that doesn't affect code functionality or readability. Both spellings are acceptable variants. The rules state not to make comments that are obvious or unimportant, and this seems to fall into that category. The inconsistency could potentially cause confusion or make the code look less polished. Some style guides do care about consistent spelling. While consistency is good, this is an example file and the meaning is perfectly clear either way. This is too minor to warrant a PR comment. Delete this comment as it points out an extremely minor stylistic issue that doesn't meaningfully impact code quality or understanding.
Workflow ID: wflow_7uuS3DOXJVgfzZbN
You can customize 
- Add { once: true } to external signal abort listeners in BamlStream
- Add { once: true } to internal abort controller listeners
- Add { once: true } to ReadableStream abort listeners
- Update NextJS API route examples with { once: true }
- Update TypeScript examples with { once: true }
- Update integration tests with { once: true }
This prevents memory leaks by automatically removing event listeners
after they fire once, which is the expected behavior for abort events.
Improves performance and ensures proper resource cleanup in long-running
applications with many concurrent requests.
|
hi @tomaszkiewicz! We're at a team offsite this week, so you'll likely get delayed responses (sometime over the weekend). Hope thats ok! Will share how to run tests shortly (by EOD). I think there's 2 things that would likely influence our design here:
(reason being, it might be sufficient to kill the stream in TS, but the network request is still running under the hood in BAML, so it may not be what the user expects) (this is all w/o reading the PR yet btw) [PS this is amazing work, and got a shout out at our offsite :)] |
|
@cg-jl is going to take a look |
There was a problem hiding this comment.
LGTM, thank you! I'll be using this to implement cancellation in the Playground.
I've annotated a couple of nitpicks we might want to review later.
The main nitpick is:
Why Option<CancellationToken> and not just CancellationToken? Probably can make AI fix that for us, but if there's a real reason behind it, I'd like to keep it that way.
There was a problem hiding this comment.
nit: I'm not sure this is needed; I'll take a look after things build.
There was a problem hiding this comment.
I'm guessing this is for local testing since you weren't able to kick CI, so I'll remove it.
There was a problem hiding this comment.
This doc looks out of place, probably splitting the docs into each language client is enough. I don't know how much of these works right now, which would be good annotating.
There was a problem hiding this comment.
Although an interesting output from an LLM, this does not seem helpful aside from keeping an LLM up to date on the status. "Performance" tests don't have a real target besides not deadlocking.
I'll remove this.
There was a problem hiding this comment.
This looks totally out of place, since it's just testing the core runtime cancellation functionality. Safe to remove it.
This is my first take on implementing support for AbortController for Typescript.
Details of feature implemented in the docs file in the commit.
It's still work in progress as I was unable to run integration testing on my fork yet, however I'm curious of your opinion about this implementation.
Important
Adds abort functionality to BAML streams using AbortController, with examples and tests.
BamlStreaminstream.tsusingAbortController.stream.abort()or externalAbortSignal.isAbortedstatus.AbortError,AbortSignal, andAbortControllerinindex.ts.BamlCallOptionswithsignalparameter instream_client.ts.abort-controller-example.tsfor TypeScript usage.AbortControllerExample.tsxfor React usage.route.tsfor Next.js API route example.abort-controller.test.tsto verify abort functionality in integration tests.This description was created by
for 224193d. You can customize this summary. It will automatically update as commits are pushed.