AbortController

[ilan-gold]

Fixes #170. I'm opening this as a draft so you may comment on the API structure here. I return empty arrays when the request is aborted - is there an edge case I am missing here? I can also raise an error, but I don't know if we want that error propagating out of the library. Let me know!

[ilan-gold]

@kylebarron I think this is ready for testing, if you want to give it a whirl, with the new release of deck.gl - changing your package.json entry to ilan-gold/geotiff.js#ilan-gold/signal_abort_controller should do the trick. I'm opening it up to review now for @constantinius to take a look at.

[constantinius]

Very nice PR!

For a merge, I'd hope that you are able to provide at least one test (if that is possible) and also some documentation (a small section in the Readme should be enough).

[ilan-gold]

@constantinius Just added some docs and testing.

[ilan-gold]

@constantinius Just resolved the conflict.

[constantinius]

@ilan-gold

One last nitpick: In my opinion the readRasters should throw an error when it was aborted, or at least return null (I'm not sure about the conventions here but the MDN docs say that fetch raises an exception upon abort).

[ilan-gold]

@constantinius Should the other methods do this as well? We would change from returning some "empty" object to raising an error.

[constantinius]

@ilan-gold
Which methods do you mean, exactly? Is there a way to let the exceptions bubble from the source itself? Then the problem would be solved in a single spot, I think.

[ilan-gold]

I was referring to readRgb mainly but I think that calls readRasters - my bad. I will update to throw an error.

[ilan-gold]

@constantinius Doing this now, but realizing DOMException does not exist in Node. Should we throw a different error?

[ilan-gold]

Just reading your other comment, we could return null. We could also throw a generic error (I like this option).

[constantinius]

My thought was to use whatever exception is thrown in the async operation when aborted, so that we don't have to deal with it and just let that exception bubble up

[ilan-gold]

@constantinius That is ok with me but we won't be able to test this if we let the exception bubble up since all the tests run in Node, where the AbortSignal API is not available. We could use node-fetch, but that would then be the default fetch for all tests. I think it would be ok to just use a generic Error in all cases, especially since we would need to something like

// Re-use the thrown AbortError.
    let abortException;

    // determine if we are the thread to start the requests.
    if (this.blockIdsAwaitingRequest) {
      ...
            catch (err) {
              if (err.name === 'AbortError') {
                abortException = err;
                this.blocks.delete(id);
                this.blockRequests.delete(id);
                this.signals.delete(id);
                this.abortedBlockIds.add(id);
     ...
    // Some of the blocks were cancelled by a signal (AbortController)
    if (blocks.some((i) => !i)) {
      // But not by this fetch's signal
      if (signal && !signal.aborted) {
        allBlockIds.forEach((blockId) => {
          if (this.abortedBlockIds.has(blockId)) {
            const request = this.requestData(
              blockId * this.blockSize, this.blockSize,
            );
            this.blockRequests.set(blockId, (async () => {
              const response = await request;
              const t = Math.min(this.blockSize, response.data.byteLength);
              const data = response.data.slice(0, t);
              this.abortedBlockIds.delete(blockId);
              this.blocks.set(blockId, {
                data,
                offset: response.offset,
                length: data.byteLength,
                top: response.offset + t,
              });
            })());
            abortedBlocksToBeRequested.push(this.blockRequests.get(blockId));
          }
        });
      // Blocks were cancelled by this signal.
      } else if (signal && signal.aborted) {
        throw abortException;
      }

to reuse the exception.

[kylebarron]

since all the tests run in Node, where the AbortSignal API is not available

By the way, it should be possible to run tests in a browser environment outside of Node. deck.gl does it using XVFB. Though since I believe geotiff.js is intended to run in both browser and Node the point is a little moot.

[constantinius]

@ilan-gold I see. Then raising a normal Error is best!

@kylebarron I'm open for new ideas to test!

[ilan-gold]

@constantinius I updated the error generation but the tests log a warning that is both cryptic and hard to solve (if you google "UnhandledPromiseRejectionWarning chai" there are a lot of ways to handle this but none seemed to work for me). I can comment out the test if you wish. Otherwise, I think this is ready.

[constantinius]

@ilan-gold

I reworked the whole "sources" part of geotiff.js in #94 and chose to directly incorporate the AbortController. I re-used a lot of stuff from your PR (which is effectively superseded). I would very much welcome if you could try it out and comment there!

[ilan-gold]

Yes of course @constantinius!!!! Anything to help out, much appreciated :) I'll give it a whirl.

[constantinius]

@ilan-gold this whole topic was integrated in PR #94

I got a lot of inspiration from this PR. Please have a look at the code and tell me if there is still some part missing.

Otherwise I think we can close this PR.