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

PGCopyOutputStream fails on close() after endCopy() #1574

Closed
virtual-machinist opened this issue Sep 22, 2019 · 5 comments
Closed

PGCopyOutputStream fails on close() after endCopy() #1574

virtual-machinist opened this issue Sep 22, 2019 · 5 comments

Comments

@virtual-machinist
Copy link
Contributor

@virtual-machinist virtual-machinist commented Sep 22, 2019

I'm submitting a ...

  • bug report
  • feature request

Describe the issue
PGCopyOutputStream fails with "java.io.IOException: Ending write to copy failed." if I call endCopy() method before closing the stream in try-with-resources block.

Driver Version?
42.2.8

Java Version?
OpenJDK 11.0.4

OS Version?
Xubuntu 18.04.3 LTS

PostgreSQL Version?
9.6.12

To Reproduce

  1. Create a table foo table with one varchar column (eg. CREATE TABLE foo (bar VARCHAR(3) PRIMARY KEY))
  2. Create a JUnit5 test class, where the DB DataSource is initialized in dataSource field
  3. Add this test method -
  @Test
  void copyFails() throws SQLException, IOException {
    String sql = "COPY foo FROM STDIN WITH (FORMAT csv, ENCODING 'UTF-8')";
    try (Connection connection = dataSource.getConnection()) {
      PGConnection pgConnection = connection.unwrap(PGConnection.class);
      try (PGCopyOutputStream out = new PGCopyOutputStream(pgConnection, sql);
           PrintWriter writer = new PrintWriter(out, true, StandardCharsets.UTF_8)) {
        writer.println("foo");
        writer.println("bar");
        writer.println("baz");
        long rowsWritten = out.endCopy();
        Assertions.assertEquals(3, rowsWritten);
      } // boom!
    }
  }
  1. Should fail on closing the stream even after a successful copy operation.

Expected behaviour
Should close the stream without throwing an exception. Should check if the copy operation is active before trying to end it the second time.

@virtual-machinist

This comment has been minimized.

Copy link
Contributor Author

@virtual-machinist virtual-machinist commented Sep 22, 2019

This issue was previously reported in the mailing list in January 2019 and there is an open PR #1387 by @sehrope with a proposed solution. However his solution introduces breaking changes in the API. Although I cannot disagree that public APIs of PGCopyOutputStream/PGCopyInputStream require overhaul (every CopyIn method implementation in PGCopyOutputStream throws an NPE after the stream is closed), this can be done later, together with possibly other incompatible changes in the driver.

Right now I propose checking if the copy operation is active before ending it for the second time. Similar approach is done in PGCopyInputStream. I can write a PR with the change if needed, provided it gets merged and released faster.

@davecramer

This comment has been minimized.

Copy link
Member

@davecramer davecramer commented Sep 23, 2019

For now I would be fine with a PR that stopped the error

@virtual-machinist

This comment has been minimized.

Copy link
Contributor Author

@virtual-machinist virtual-machinist commented Sep 24, 2019

So basically my fix is ready. In theory we could fix all CopyIn implementation methods to return something meaningful after copy has ended (getFieldCount(), getFieldFormat(), etc. - they are known at the time of stream construction once the delegate CopyIn is created), or at least throw something other than a NullPointerException (other methods that require writing to backend), but I'd make that a separate effort.

@shjamloki

This comment has been minimized.

Copy link

@shjamloki shjamloki commented Nov 12, 2019

We are also facing this issue frequently. Is there a timeline for this fix ?

davecramer added a commit that referenced this issue Nov 12, 2019
…1575)

* Fix: check if active before close() (#1574)

* Fix: NPEs on flush() and isActive() if copy has ended

* Tests for close(), flush(), isActive()
@davecramer

This comment has been minimized.

Copy link
Member

@davecramer davecramer commented Nov 15, 2019

Fixed with #1575

@davecramer davecramer closed this Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.