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

Download.dataset partially downloads ( corrupts) files? #744

Closed
Tracked by #1544
travatine opened this issue Jun 22, 2020 · 3 comments · Fixed by #2167
Closed
Tracked by #1544

Download.dataset partially downloads ( corrupts) files? #744

travatine opened this issue Jun 22, 2020 · 3 comments · Fixed by #2167
Assignees
Labels
bug Something isn't working good first issue Good for newcomers needs-ported Indicates that a PR needs to be ported (master - lts-incremental) priority-medium Not functioning - next quarter if capacity permits

Comments

@travatine
Copy link

Hi,

Download.dataset() uses a "fs.createWriteStream()" to create a writable stream.
The writeablestream end() function is not explicitly being called.

For small files ( less than 64K), this results in incomplete files / data loss / corruption.

 public static async dataSet(session: AbstractSession, dataSetName: string, options: IDownloadOptions = {}): Promise<IZosFilesResponse> {
...
 const writeStream = IO.createWriteStream(destination);

            // Use specific options to mimic ZosmfRestClient.getStreamed()
            const requestOptions: IOptionsFullResponse = {
                resource: endpoint,
                reqHeaders,
                responseStream: writeStream,
                normalizeResponseNewLines: !options.binary,
                task: options.task
            };

            // If requestor needs etag, add header + get "response" back
            if (options.returnEtag) {
                requestOptions.reqHeaders.push(ZosmfHeaders.X_IBM_RETURN_ETAG);
                requestOptions.dataToReturn = [CLIENT_PROPERTY.response];
            }

            const request: IRestClientResponse = await ZosmfRestClient.getExpectFullResponse(session, requestOptions);

Might it possible for "end()" to be called on "writeStream" , after the "getExpectFullResponse" call, so the stream data is written to the file immediately?

Thanks

@KevinLoesch1 KevinLoesch1 added for-review To be reviewed in an Eng & Prod Mgmt meeting bug Something isn't working labels Sep 23, 2022
@github-actions
Copy link

Thank you for creating a bug report. If you haven't already, please ensure you have provided steps to reproduce it and as much context as possible.

@gejohnston gejohnston added the priority-medium Not functioning - next quarter if capacity permits label Oct 17, 2022
@zFernand0 zFernand0 added good first issue Good for newcomers needs-ported Indicates that a PR needs to be ported (master - lts-incremental) and removed for-review To be reviewed in an Eng & Prod Mgmt meeting labels Dec 21, 2022
@Shofiya2003
Copy link

Shofiya2003 commented May 30, 2023

Hello
I would like to work on this issue. Would suggest a solution as soon as possible.
@zFernand0

@travatine
Copy link
Author

Hi @Shofiya2003 ,

For some additional context,
I was trying to download a dataset , using Download.dataset(),
Then immediately reading the local file.

The problem was that, the download.datset() function returns before the data is actually written to the file. (Because end() was not being called?)

This is similar to the problem described in ,
https://stackoverflow.com/questions/61913033/node-js-closing-a-file-after-writing .

Adding a call to end() on the write stream, and waiting for the finish event might help:
E.g.

writeStream.end();
await once(writeStream, 'finish');

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers needs-ported Indicates that a PR needs to be ported (master - lts-incremental) priority-medium Not functioning - next quarter if capacity permits
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants