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

Promise doesn't fulfill on destroyed body stream #2370

Open
Rychu-Pawel opened this issue Aug 2, 2024 · 1 comment
Open

Promise doesn't fulfill on destroyed body stream #2370

Rychu-Pawel opened this issue Aug 2, 2024 · 1 comment

Comments

@Rychu-Pawel
Copy link
Contributor

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
Copy link
Contributor Author

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.

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

No branches or pull requests

1 participant