GMOD / tabix-js

Read Tabix-indexed files, either with .tbi or .csi indexes, in node or the browser
MIT License
14 stars 5 forks source link

Abort signal is not passed to chunkCache and subsequent fetch requests #143

Closed tuner closed 10 months ago

tuner commented 10 months ago

Hi,

Although tbiIndexed.getLines accepts an AbortSignal, it's not propagated to chunkCache and thus, the fetch request are not aborted.

https://github.com/GMOD/tabix-js/blob/a7004eb4b356aa6151fc3602bb73c47d92be8834/src/tabixIndexedFile.ts#L189

It seems that the third parameter, the signal is missing (?)

cmdcolin commented 10 months ago

i think you might be right here. i've found it really hard to test abort signal code so it sometimes has bugs...thanks for catching. PR to add this here https://github.com/GMOD/tabix-js/pull/144

cmdcolin commented 10 months ago

note that this signal, once pr is merged, will then go through abortable-promise-cache which we've done our best to make sure works, but has some somewhat complex behavior of it's own (e.g. if multiple requests are made for the same resource, it only aborts if all of them abort).

I will release a patch version of tabix and then you can try it out though!

cmdcolin commented 10 months ago

published as 1.5.12!

tuner commented 10 months ago

Hi Colin. Thanks for the prompt reply!

There's another issue that I didn't catch. signal is declared but never assigned! After fixing that in my node_modules folder (I was lazy), it started to behave as expected:

image

I throttled the network in Devtools so that it's easier to abort the web requests 😃

tuner commented 10 months ago

note that this signal, once pr is merged, will then go through abortable-promise-cache which we've done our best to make sure works, but has some somewhat complex behavior of it's own (e.g. if multiple requests are made for the same resource, it only aborts if all of them abort).

That behavior sounds reasonable. If someone needs the resource, then it has to be fetched. There's no overhead.

cmdcolin commented 10 months ago

nice catch, added another point release with that fix (v1.5.13)

tuner commented 10 months ago

Thanks! It works now.

I'll try to make a PR next time!