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

usage with stream.finished no longer working in node v12.16.1 #89

Closed
shusson opened this issue Mar 2, 2020 · 4 comments
Closed

usage with stream.finished no longer working in node v12.16.1 #89

shusson opened this issue Mar 2, 2020 · 4 comments

Comments

@shusson
Copy link

shusson commented Mar 2, 2020

We previously used the following pattern:


    const copyStream = client.query(
        pgCopy.from(
            `COPY MY_TABLE FROM STDIN WITH (FORMAT 'csv')`
        )
    );

    const csv: string[] = getCsvStrings();

    const input = new Readable({
        objectMode: true,
        read() {
            const item = csv.shift();

            if (!item) {
                this.push(null);
            } else {
                this.push(`${item}\n`);
            }
        }
    });

    await pipeline(input, copyStream);
    await finished(copyStream);

In node 12.16.1, await finished(copyStream); now waits indefinitely.

The pattern we used was based on the understanding that #21 and #86 would be solved by using nodes finished method. Which was working until we recently upgraded to 12.16.1. Last known working version was 12.14.0, which we have reverted to now until we figure out how to fix this.

@shusson shusson changed the title stream.finished no longer working in node v12.16.1 usage with stream.finished no longer working in node v12.16.1 Mar 2, 2020
@shusson
Copy link
Author

shusson commented Mar 2, 2020

It looks like node keeps changing the behviour of the streams api e.g nodejs/node#32032 nodejs/node#29699.

For now we are going with something a lot simpler and as recommended in the readme:

    await new Promise((resolve, reject) => {
        copyStream.on("error", reject);
        copyStream.on("end", resolve);
        input.on("error", reject);
        input.pipe(copyStream);
    });

@shusson shusson closed this as completed Mar 2, 2020
@jeromew
Copy link
Collaborator

jeromew commented Mar 2, 2020

thanks for the report and finding a solution that works for you. This module is a bit particular because of how it needs to pipe into the postgres connection stream. It could probably be modified to have a more standard semantic but it needs work.

@jeromew jeromew reopened this May 11, 2020
@jeromew
Copy link
Collaborator

jeromew commented May 11, 2020

@shusson I re-open this issue because I would be very grateful if you could test again with version 4.0.0 that was just published. It should behave correctly with the finished and pipeline API.

don't hesitate to tell me if you don't have time to do this or if you don't use this module anymore. Thank you

the writable copy-from now finishes on the standard 'finish' and not on the writable non-standard 'end'

@shusson
Copy link
Author

shusson commented May 11, 2020

@jeromew our tests pass with 4.0.0 and using pipeline 💚.

Our code looks something like this now:

    const copyStream = client.query(
        pgCopy.from(
            `COPY MY_TABLE FROM STDIN WITH (FORMAT 'csv')`
        )
    );

    const input = new Readable({
       ...
    });

    await pipeline(input, copyStream);

Thanks for the update.

@shusson shusson closed this as completed May 11, 2020
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