sindresorhus / got

🌐 Human-friendly and powerful HTTP request library for Node.js
MIT License
14.32k stars 938 forks source link

Promise doesn't fulfill on destroyed body stream #2370

Open Rychu-Pawel opened 3 months ago

Rychu-Pawel commented 3 months ago

I'm working on a new feature to resume interrupted file transfers at a later time. For this feature to work, I need to ensure that the value in my uploadedBytes variable matches exactly what the API, that is receiving this file, has written to the disk. However, I'm facing issues with my current implementation.

Simplified code:

this.#fileHandle = await fs.open(this.#fullFilePath, fsConsts.O_RDONLY);

this.#fileReadStream = this.#fileHandle.createReadStream({
    start: this.chunkStartPosition,
    end: this.getChunkEndPosition(),
});

let uploadedBytes = 0;

const ongoingRequestPromise = gotChunkInstance.post(this.#uploadUrl, {
    body: this.#fileReadStream,
    headers: {
        'content-length': this.chunkLengthInBytes.toString(),
    },
});

ongoingRequestPromise.on(`uploadProgress`, progress => {
    uploadedBytes = progress.transferred;
});

this.#abortSignal.addEventListener(`abort`, () => {
    ongoingRequestPromise.cancel();
});

await ongoingRequestPromise;

Server-Side Pseudocode

The server code handling this request can be represented with the following pseudocode:

SaveFile(stream requestStream, CancellationToken cancellationToken)
{
  while(!cancellationToken.cancelled) 
  {
    WriteBytesToFile(requestStream.read())
  }
}

Problem

When aborting the upload with this.#abortSignal, some bytes reported by GOT's uploadProgress event are never saved to the server's disk. As ongoingRequestPromise.cancel() closes the connection, I assume these bytes might be lost in various places, such as in the network card buffer or the server's buffer, or discarded because the server's CancellationToken was cancelled before they could be written to the disk.

New approach

To address this, I decided to gracefully destroy the file stream, assuming that if I manage to close the file stream gracefully, GOT will read all buffered bytes from the stream, send them, and then fulfill the promise. Although the abort operation won't be immediate, in my scenario, this is acceptable as long as I'm sure that the server safely writes all sent bytes to its disk and my uploadedBytes variable remains accurate.

Nodified code

this.#fileHandle = await fs.open(this.#fullFilePath, fsConsts.O_RDONLY);

this.#fileReadStream = this.#fileHandle.createReadStream({
    start: this.chunkStartPosition,
    end: this.getChunkEndPosition(),
});

// This event handler is new
this.#abortSignal.addEventListener(`abort`, () => {
    // I tried every combination of below but no luck here
    this.#fileReadStream?.push(null);
    this.#fileReadStream?.close();
    this.#fileReadStream?.emit(`end`);
    this.#fileReadStream?.unpipe();
    this.#fileReadStream?.destroy();
});

let uploadedBytes = 0;

const ongoingRequestPromise = gotChunkInstance.post(this.#uploadUrl, {
    body: this.#fileReadStream,
    headers: {
        'content-length': this.chunkLengthInBytes.toString(),
    },
});

ongoingRequestPromise.on(`uploadProgress`, progress => {
    uploadedBytes = progress.transferred;
});

await ongoingRequestPromise;

Issue

This correctly stops the upload gracefully, but unfortunately, ongoingRequestPromise is never fulfilled. If I change my abort signal handler to this.#fileReadStream?.destroy(new Error());, an error is thrown, and the connection is closed immediately, making it no different from the initial approach using ongoingRequestPromise.cancel();.

Questions

  1. Is this a bug?
  2. Is there a different way to achieve what I want?

I'm using GOT 14.2.x and NodeJS 20.x.x

Thank you for your help!

Rychu-Pawel commented 3 months ago

When refactoring my code for this feature I forgot to change the header from content-length to 'Transfer-Encoding': 'chunked'. With that change, GOT gracefully closes the request, and all data is correctly processed by the server.