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

Files Fail to Download #10

Closed
radamson opened this issue Jan 10, 2023 · 4 comments
Closed

Files Fail to Download #10

radamson opened this issue Jan 10, 2023 · 4 comments

Comments

@radamson
Copy link
Contributor

radamson commented Jan 10, 2023

It appears that a recent commit caused a regression resulting in empty bulk data download files. I was able to use git bisect to trace the regression to commit 8b642be. Upon further investigation I noticed that the TypeScript is not transpiled at every step, which means the error could be in a different commit. 3f8d2e3 needs a minor modification to transpile (adding file?: string to the LoggingOptions type), but otherwise works.

470f6f0 appears to be the true culprit.

Here is the command I used:

node . -f https://bulk-data.smarthealthit.org/eyJlcnIiOiIiLCJwYWdlIjoxMDAwMCwiZHVyIjoxMCwidGx0IjoxNSwibSI6MSwic3R1IjozLCJkZWwiOjB9/fhir -g 048d4683-703a-4311-963d-48e515a6372b

The console will show:

Kick-off started
Kick-off completed
Bulk Data export started
Bulk Data export completed in 10 seconds

Downloading exported files: ▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉▉ 100%
          Downloaded Files: 12 of 12
            FHIR Resources: 2,185
               Attachments: 0
           Downloaded Size: 178.5 kB
         Uncompressed Size: 2.7 MB
         Compression ratio: 1/16

Download completed in 2 seconds

But, the files in the download directory will be empty:

> ls -al downloads

... 480 Jan 10 17:26 .
... 704 Jan 10 17:26 ..
... 0 Jan 10 17:26 1.CarePlan.ndjson
... 0 Jan 10 17:26 1.Claim.ndjson
...

Tested with node v17.19.1.

@radamson
Copy link
Contributor Author

radamson commented Jan 10, 2023

I suspect this may be an issue with versions of node from v16.10.0 up to, but not including, v18.0.0 based on nodejs/node#40191. Versions higher than v18.0.0 suffer from Issue #9. Node 16 no longer receives active support and will only receive security updates until Sept 2023.

EDIT: Still seeing empty downloads on Node 16.9.1 and 16.9.0

@radamson
Copy link
Contributor Author

More context based on some investigation of the changes in 470f6f0.

stream.pipeline

stream.pipeline() will call stream.destroy(err) on all streams except:

    Readable streams which have emitted 'end' or 'close'.
    Writable streams which have emitted 'finish' or 'close'.

stream.pipeline() leaves dangling event listeners on the streams after the callback has been invoked. In the case of reuse of streams after failure, this can cause event listener leaks and swallowed errors.

with the switch to pipeline, I believe the stringify stream is now being destroyed before it can be used by writeToDestintation. To properly use pipeline, I believe you'd want something like createWriteToDestinationStream which returns a writeable stream that can be used as part of the pipeline and then destroyed once the pipeline is completed.

@vlad-ignatov
Copy link
Contributor

Thanks @radamson, your investigation was very helpful! I pushed some changes that should hopefully fix this issue. The NodeJS version shouldn't matter. Can you verify if it works for you now?

@radamson
Copy link
Contributor Author

Tested out the updated main branch and the issue appears to be resolved. Thanks!

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

2 participants