Derby
  1. Derby
  2. DERBY-4531

Client setCharacterStream closes its Reader argument stream in finalizer

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.2.1.6, 10.2.2.0, 10.3.1.4, 10.3.2.1, 10.3.3.0, 10.4.1.3, 10.4.2.0, 10.5.1.1, 10.5.2.0, 10.5.3.0, 10.6.1.0
    • Fix Version/s: 10.7.1.1
    • Component/s: Network Client
    • Issue & fix info:
      Repro attached
    • Bug behavior facts:
      Embedded/Client difference, Regression

      Description

      The javadoc for PreparedStatement.setCharacterStream does not specify that the stream passed in will be closed, only that it will read to the number of characters specified.
      For the embedded driver, the stream is not closed after execution; the client driver, however, will close the stream when the internal stream object EncodedInputStream is garbage collected, which can happen any time after the statement has been executed.
      I am not sure this a bug vs. the JDBC specification, but it would be nice to harmonize client and embedded behavior on this.

      1. Repro.java
        14 kB
        Dag H. Wanvik
      2. derby-4531-1a-test_workaround.diff
        2 kB
        Kristian Waagan
      3. derby-4531.diff
        0.5 kB
        Dag H. Wanvik
      4. derby-4531b.diff
        3 kB
        Dag H. Wanvik
      5. derby-4531b.stat
        0.2 kB
        Dag H. Wanvik

        Issue Links

          Activity

          Hide
          Kathey Marsden added a comment -

          reopen to add derby_backport_reject label

          Show
          Kathey Marsden added a comment - reopen to add derby_backport_reject label
          Hide
          Dag H. Wanvik added a comment -

          Since this patch removes the regression, I don't believe we require a release note, so unsetting that flag, and closing.

          Show
          Dag H. Wanvik added a comment - Since this patch removes the regression, I don't believe we require a release note, so unsetting that flag, and closing.
          Hide
          Dag H. Wanvik added a comment -

          Committed derby-4531b as svn 987331.

          Show
          Dag H. Wanvik added a comment - Committed derby-4531b as svn 987331.
          Hide
          Dag H. Wanvik added a comment -

          Attaching a new rev of the patch which makes the client driver behave like the embedded driver in this respect: derby-4531b.

          It adds a regression test. At least on my box with Sun JDK 1.6.0_16, the new test fails for the client driver prior to the patch proper, but as always when explicitly calling gc, YMMV. With the full patch the test should work reliably in any case.

          Running full regressions.

          Show
          Dag H. Wanvik added a comment - Attaching a new rev of the patch which makes the client driver behave like the embedded driver in this respect: derby-4531b. It adds a regression test. At least on my box with Sun JDK 1.6.0_16, the new test fails for the client driver prior to the patch proper, but as always when explicitly calling gc, YMMV. With the full patch the test should work reliably in any case. Running full regressions.
          Hide
          Dag H. Wanvik added a comment -

          Apparently, there is no consensus among the vendors. I propose to commit the current patch for now to get alignment between the drivers. Then, if we later decide to move to the restrictive behavior if/when this is clarified in the spec, we could do it to both drivers.

          Show
          Dag H. Wanvik added a comment - Apparently, there is no consensus among the vendors. I propose to commit the current patch for now to get alignment between the drivers. Then, if we later decide to move to the restrictive behavior if/when this is clarified in the spec, we could do it to both drivers.
          Hide
          Dag H. Wanvik added a comment -

          Lance informs me that the spec is silent on this, but that the JDBC 3.0 book says:

          "length - the number of bytes to be read from the stream & sent to the database. Note that if the stream contains more or less bytes than are specified in length, an exception is thrown".

          If we want to go this way, we would need to change both drivers, increasing potential for breaking existing apps that supply too long streams.
          Any thoughts on which way to go? Lance said he's check what other vendors do.

          Show
          Dag H. Wanvik added a comment - Lance informs me that the spec is silent on this, but that the JDBC 3.0 book says: "length - the number of bytes to be read from the stream & sent to the database. Note that if the stream contains more or less bytes than are specified in length, an exception is thrown". If we want to go this way, we would need to change both drivers, increasing potential for breaking existing apps that supply too long streams. Any thoughts on which way to go? Lance said he's check what other vendors do.
          Hide
          Dag H. Wanvik added a comment -

          No, not yet. If I don't get any, the community should decide what to do with this one. I am inclined to commit it (i.e. the driver should not close the stream).

          Show
          Dag H. Wanvik added a comment - No, not yet. If I don't get any, the community should decide what to do with this one. I am inclined to commit it (i.e. the driver should not close the stream).
          Hide
          Kristian Waagan added a comment -

          Any feedback that will allow us to move forward on this issue yet?

          Show
          Kristian Waagan added a comment - Any feedback that will allow us to move forward on this issue yet?
          Hide
          Dag H. Wanvik added a comment - - edited

          Uploading a patch that removes the finalizer close. Regrressions ran ok. I have asked Lance to comment on the JDBC semantics, and I intend to hold back commit until we agree that this change is the right way to go.
          It does change behavior, so I mark it with release note needed.

          Show
          Dag H. Wanvik added a comment - - edited Uploading a patch that removes the finalizer close. Regrressions ran ok. I have asked Lance to comment on the JDBC semantics, and I intend to hold back commit until we agree that this change is the right way to go. It does change behavior, so I mark it with release note needed.
          Hide
          Rick Hillegas added a comment -

          Some additional discussion of this issue can be found on this email thread: http://old.nabble.com/instability-in-StreamingColumnTest-tt28369553.html#a28369553

          Show
          Rick Hillegas added a comment - Some additional discussion of this issue can be found on this email thread: http://old.nabble.com/instability-in-StreamingColumnTest-tt28369553.html#a28369553
          Hide
          Kristian Waagan added a comment -

          Attached patch 1a, which is a work-around for an intermittent test failure caused by the client driver closing the stream in the finalizer.

          Committed to trunk with revision 938972.
          I plan to back-port this to the 10.6 branch.

          Show
          Kristian Waagan added a comment - Attached patch 1a, which is a work-around for an intermittent test failure caused by the client driver closing the stream in the finalizer. Committed to trunk with revision 938972. I plan to back-port this to the 10.6 branch.
          Hide
          Dag H. Wanvik added a comment -

          Attaching a repro.

          Show
          Dag H. Wanvik added a comment - Attaching a repro.
          Hide
          Dag H. Wanvik added a comment -

          Derby will throw if the stream does not hold the exact amount of data specified in setCharacterStream, so one might argue that keeping the stream open doesn't accomplish much. Still, I think that since the API does not prescribe closing the stream, a driver should refrain from doing it.

          Show
          Dag H. Wanvik added a comment - Derby will throw if the stream does not hold the exact amount of data specified in setCharacterStream, so one might argue that keeping the stream open doesn't accomplish much. Still, I think that since the API does not prescribe closing the stream, a driver should refrain from doing it.
          Hide
          Dag H. Wanvik added a comment -

          This behavior was introduced in 10.2 with svn 414562, so marking
          as regression until somebody convinces me this is not a bug

          Show
          Dag H. Wanvik added a comment - This behavior was introduced in 10.2 with svn 414562, so marking as regression until somebody convinces me this is not a bug
          Hide
          Dag H. Wanvik added a comment -

          The client driver does not similarly close an InputStream given as as argument to setBinaryStream.

          Show
          Dag H. Wanvik added a comment - The client driver does not similarly close an InputStream given as as argument to setBinaryStream.

            People

            • Assignee:
              Dag H. Wanvik
              Reporter:
              Dag H. Wanvik
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development