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

Imperative streaming API does not update the ITaskWithStatus correctly #1920

Open
phaumer opened this issue May 28, 2021 · 3 comments
Open
Labels
bug Something isn't working priority-low Legit issue but cosmetic or nice-to-have severity-low Bug that makes the usage of the Zowe less convenient but doesn't impact key use cases

Comments

@phaumer
Copy link
Member

phaumer commented May 28, 2021

The request() method in packages/rest/src/client/AbstractRestClient.ts implements a streaming API for downloading and uploading files. For that the caller can provide a ITaskWithStatus with the request options to monitor the progress. However, if used programmatically in the SDK via a nodejs app that task cannot be used to decide if the file is successfully downloaded and available for reading. This would be a very important requirement for consumers who want to download content such as data sets to a file and then process these files.

The problem is that the task is updated with TaskStage.COMPLETE at the beginning of the onEnd() method before even calling the stream.onEnd(). onEnd() only indicates that all the content is being received. However, that does not mean that at this point all the content has been written and flushed to a file that was used for the stream.

This is only the case when the finish event is called, which only happens after the stream.end(). See the nodejs docs here https://nodejs.org/docs/latest-v12.x/api/stream.html#stream_event_finish. The impact is that code that gets called right after the request finishes and task is changed to complete will still find an incomplete file that does not have all the content.

I propose to implement also the writer.on('finish') handler that then updates the task to complete.

@t1m0thyj
Copy link
Member

t1m0thyj commented Jun 8, 2021

Thanks for reporting this, could you provide a minimal repro that demonstrates the data loss happening?

The documentation you linked to regarding stream event "finish" seems to only apply to the Writable stream class. For the http/https ClientRequest streams used by AbstractRestClient, the documentation states:

Until the data is consumed, the 'end' event will not fire.

This leads me to believe that relying on the stream event "end" should be fine for the Imperative use case.

@phaumer
Copy link
Member Author

phaumer commented Jun 15, 2021

Ok, good idea. Will work on that.

@t1m0thyj t1m0thyj added bug Something isn't working priority-low Legit issue but cosmetic or nice-to-have severity-low Bug that makes the usage of the Zowe less convenient but doesn't impact key use cases labels Nov 29, 2022
@t1m0thyj
Copy link
Member

t1m0thyj commented Nov 29, 2022

Upon further investigation, I believe this is a valid issue when a response stream is used with the Imperative REST client.

We define the task stage as complete prematurely - before the response stream is ended:
https://github.com/zowe/imperative/blob/268eceb3faa657f3c96741753d1acfa28c26a9a6/packages/rest/src/client/AbstractRestClient.ts#L653-L660

@awharn awharn transferred this issue from zowe/imperative Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority-low Legit issue but cosmetic or nice-to-have severity-low Bug that makes the usage of the Zowe less convenient but doesn't impact key use cases
Projects
Status: Low Priority
Development

No branches or pull requests

2 participants