Derby
  1. Derby
  2. DERBY-2017

Client driver can insert and commit partial data when a LOB stream throws IOException or does not match the specified length

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.2.1.6
    • Fix Version/s: 10.6.1.0
    • Component/s: JDBC, Network Client
    • Labels:
      None
    • Urgency:
      Normal
    • Issue & fix info:
      High Value Fix, Repro attached
    • Bug behavior facts:
      Data corruption, Wrong query result

      Description

      When a LOB stream throws an exception or does not match the specified length, the client driver does not raise an exception until it has finished executing the statement. Therefore, the statement will be executed (and possibly committed) on the server even though the client reports that the statement failed.

      1. derby-2017-5a-binary_tests.diff
        22 kB
        Kristian Waagan
      2. derby-2017-4a-remove_test_workaround.diff
        1 kB
        Kristian Waagan
      3. derby-2017-3c-fix.diff
        57 kB
        Kristian Waagan
      4. derby-2017-3b-fix.diff
        56 kB
        Kristian Waagan
      5. derby-2017-3a-fix.stat
        0.9 kB
        Kristian Waagan
      6. derby-2017-3a-fix.diff
        41 kB
        Kristian Waagan
      7. derby-2017-2b-regression-test.diff
        36 kB
        Kristian Waagan
      8. derby-2017-stream_status_preview.diff
        45 kB
        Kristian Waagan
      9. derby-2017-2a-regression_test.diff
        9 kB
        Kristian Waagan
      10. derby2017_try1.diff
        3 kB
        Mayuresh Nirhali
      11. Derby_2017_v1.diff
        2 kB
        Saurabh Vyas
      12. Derby_2017_v1.stat
        0.2 kB
        Saurabh Vyas
      13. StreamErrRepro.java
        2 kB
        Knut Anders Hatlen

        Issue Links

          Activity

          Hide
          Knut Anders Hatlen added a comment -

          Attached a repro which shows the problem when using PreparedStatement.setCharacterStream() with a stream that is longer than the specified length.

          The embedded driver aborts the query and throws "java.sql.SQLException: An IOException was thrown when reading a 'java.sql.String' from an InputStream."

          The client driver completes the query (inserts a row) and throws "java.sql.SQLException: Network protocol error: the specified size of the InputStream, parameter #1, is less than the actual InputStream length."

          The client driver should abort the the query and not insert the row.

          Show
          Knut Anders Hatlen added a comment - Attached a repro which shows the problem when using PreparedStatement.setCharacterStream() with a stream that is longer than the specified length. The embedded driver aborts the query and throws "java.sql.SQLException: An IOException was thrown when reading a 'java.sql.String' from an InputStream." The client driver completes the query (inserts a row) and throws "java.sql.SQLException: Network protocol error: the specified size of the InputStream, parameter #1, is less than the actual InputStream length." The client driver should abort the the query and not insert the row.
          Hide
          Knut Anders Hatlen added a comment -

          When this issue has been fixed, jdbcapi/CharacterStreamsTest should be updated so that it runs with the client driver.

          Show
          Knut Anders Hatlen added a comment - When this issue has been fixed, jdbcapi/CharacterStreamsTest should be updated so that it runs with the client driver.
          Hide
          Saurabh Vyas added a comment -

          When an error occurs, rolling back the transaction in network mode so as the database remains consistent.
          Handling the scenario in the same way its done in org.apache.derby.client.am.Statement.checkForStoredProcResultSetCount() which rolls back the transaction on error.

          • also modified the test CharacterStreamsTest to run under client mode.
          • derbyall & Junit tests runs fine.
          Show
          Saurabh Vyas added a comment - When an error occurs, rolling back the transaction in network mode so as the database remains consistent. Handling the scenario in the same way its done in org.apache.derby.client.am.Statement.checkForStoredProcResultSetCount() which rolls back the transaction on error. also modified the test CharacterStreamsTest to run under client mode. derbyall & Junit tests runs fine.
          Hide
          Saurabh Vyas added a comment -

          Adding fixed version to 10.2.2 also

          Show
          Saurabh Vyas added a comment - Adding fixed version to 10.2.2 also
          Hide
          Bryan Pendleton added a comment -

          Hi Saurabh,

          After the discussion on the mailing list regarding this issue I'm still
          confused. Does your change proposal make the network client configuration's
          behavior match the embedded system behavior? Or do the two configurations
          behave differently? See this part of the discussion:
          http://www.nabble.com/forum/ViewPost.jtp?post=7735323&framed=y

          I'm worried that it is incorrect to roll back the entire transaction for
          this error; that only the statement that caused the LOB problem should
          be rolled back.

          Show
          Bryan Pendleton added a comment - Hi Saurabh, After the discussion on the mailing list regarding this issue I'm still confused. Does your change proposal make the network client configuration's behavior match the embedded system behavior? Or do the two configurations behave differently? See this part of the discussion: http://www.nabble.com/forum/ViewPost.jtp?post=7735323&framed=y I'm worried that it is incorrect to roll back the entire transaction for this error; that only the statement that caused the LOB problem should be rolled back.
          Hide
          Saurabh Vyas added a comment -

          Well with this approach, the two configurations will behave differently. In embedded mode, in case of error, only the erroneous statement will make no effect on database and rest of the statements in transaction will remain unaffected by the error. Whereas in network mode, in case of error, the complete transaction will be rolled back so that the database can remain in consistent state (which is not handled currently as pointed out in repro).

          I tried searching options from DRDA specifications but the DO_NOT_CONTINUE_ON_ERROR flag provided in DSS header will not be of any use as the error is at client side. Thanks to Julo for pointing this out.

          Apart, as mentioned by Knut also, the closest we can get as long as we're using DRDA. Can we try any other approach for this.
          Comments/Suggestions.

          Show
          Saurabh Vyas added a comment - Well with this approach, the two configurations will behave differently. In embedded mode, in case of error, only the erroneous statement will make no effect on database and rest of the statements in transaction will remain unaffected by the error. Whereas in network mode, in case of error, the complete transaction will be rolled back so that the database can remain in consistent state (which is not handled currently as pointed out in repro). I tried searching options from DRDA specifications but the DO_NOT_CONTINUE_ON_ERROR flag provided in DSS header will not be of any use as the error is at client side. Thanks to Julo for pointing this out. Apart, as mentioned by Knut also, the closest we can get as long as we're using DRDA. Can we try any other approach for this. Comments/Suggestions.
          Hide
          Rick Hillegas added a comment -

          Unknown release vehicle.

          Show
          Rick Hillegas added a comment - Unknown release vehicle.
          Hide
          Kristian Waagan added a comment -

          Clearing patch available flag.
          The issue seems to be stalled. I think we need another round of effort to decide what is an acceptable approach.
          If the community reaches consensus on using the approach implemented by the existing patch, please set the patch available flag again.

          Show
          Kristian Waagan added a comment - Clearing patch available flag. The issue seems to be stalled. I think we need another round of effort to decide what is an acceptable approach. If the community reaches consensus on using the approach implemented by the existing patch, please set the patch available flag again.
          Hide
          Saurabh Vyas added a comment -

          Thanks Kristian for bringing this issue to attention and another round of discussion over it.

          Till now, according to my approach, While an error occurs, in the network mode, the complete transaction will be rolled back. This is to make sure that Data is in consistent state, but this approach also rollbacks some correct operations on database (all insert, updates & delete prior to the erroneous statement). This behavior is is different from that of the embedded mode.

          To make the consistent behavior for both embedded & network mode, there should be an mechanism by which the client should be able to communicate to server that the last operation is invalid & only this operation has to be reverted. This implies that at the time of error rather than ignoring the read error, the client need to fire another statement to revert the current changes done at server (currently client calls connection_.writeCommitSubstitute_(); ). So how should we go ahead for this approach ?

          Show
          Saurabh Vyas added a comment - Thanks Kristian for bringing this issue to attention and another round of discussion over it. Till now, according to my approach, While an error occurs, in the network mode, the complete transaction will be rolled back. This is to make sure that Data is in consistent state, but this approach also rollbacks some correct operations on database (all insert, updates & delete prior to the erroneous statement). This behavior is is different from that of the embedded mode. To make the consistent behavior for both embedded & network mode, there should be an mechanism by which the client should be able to communicate to server that the last operation is invalid & only this operation has to be reverted. This implies that at the time of error rather than ignoring the read error, the client need to fire another statement to revert the current changes done at server (currently client calls connection_.writeCommitSubstitute_(); ). So how should we go ahead for this approach ?
          Hide
          Saurabh Vyas added a comment -

          I will not be working on this anymore, so un-assigning myself

          Show
          Saurabh Vyas added a comment - I will not be working on this anymore, so un-assigning myself
          Hide
          Mayuresh Nirhali added a comment -

          This is a HIGH VALUE FIX CANDIDATE.

          I did some more investigation to understand the exact difference in client behavior compared to the embedded.
          In embedded mode, the stream is checked for its data by ReadertoUTF8Stream to see if the length specified is matching with the number of characters in the stream. An exception is raised if the length is not matched, hence we see the exception in the repro.

          Client does not check for stream data before hand and reads the stream data only when that is needed to pass onto to the server. So, by the time the lenght matching errors are caught, the DSS header and the full/partial data is sent to the server.

          I tried a different approach (attached patch) to fix this bug. The stream is read (like in embedded mode) to validate the length much before the DSS header is sent to the server.

          The patch is not ready for commit, but it solves the problem. I feel, validation of stream should be done even before the place in attached patch, something like in LocatorStream which is under development. Like in embedded, a new method 'checkSufficientData' should be implemented in a class that is equivalent to ReaderToUTF8Stream in embedded.

          I would like to see comments on this approach from experts.

          Show
          Mayuresh Nirhali added a comment - This is a HIGH VALUE FIX CANDIDATE. I did some more investigation to understand the exact difference in client behavior compared to the embedded. In embedded mode, the stream is checked for its data by ReadertoUTF8Stream to see if the length specified is matching with the number of characters in the stream. An exception is raised if the length is not matched, hence we see the exception in the repro. Client does not check for stream data before hand and reads the stream data only when that is needed to pass onto to the server. So, by the time the lenght matching errors are caught, the DSS header and the full/partial data is sent to the server. I tried a different approach (attached patch) to fix this bug. The stream is read (like in embedded mode) to validate the length much before the DSS header is sent to the server. The patch is not ready for commit, but it solves the problem. I feel, validation of stream should be done even before the place in attached patch, something like in LocatorStream which is under development. Like in embedded, a new method 'checkSufficientData' should be implemented in a class that is equivalent to ReaderToUTF8Stream in embedded. I would like to see comments on this approach from experts.
          Hide
          V.Narayanan added a comment -

          I have a few questions, pls feel free to ignore them if you think they are unrelated.

          1) By "The stream is read (like in embedded mode) to validate the length" is it meant that the
          Stream should be materialized completely in tempChars (the array used in the patch
          attached)?

          If this is true wouldn't it result in OutOfMemory problems when the stream is too large?

          2) The logic used to see that the exception must be thrown is
          a) Read length bytes (InputStream.read(byte[] b, int off, int len))
          b) Read one more byte (read()) if this returns -1 means that the stream has
          no more data.

          If the above two steps is the algorithm for the logic the following
          snippet from the API doc of InputStream.read(byte[] b, int off, int len) might be
          relevant "An attempt is made to read as many as len bytes, but a smaller
          number may be read, possibly zero. The number of bytes actually read is returned
          as an integer."

          3) Also in derby2017_try1.diff bytesToRead is no longer used. bytesToRead is
          set to the minimum of DssConstants.MAX_DSS_LEN - 6 - 4 - 1 - extendedLengthByteCount.
          I think this is the maximum chunk size that can be sent. Has derby2017_try1.diff
          been tried with a stream of size greater than 32767?

          Sorry if the above seem like random thoughts.

          Show
          V.Narayanan added a comment - I have a few questions, pls feel free to ignore them if you think they are unrelated. 1) By "The stream is read (like in embedded mode) to validate the length" is it meant that the Stream should be materialized completely in tempChars (the array used in the patch attached)? If this is true wouldn't it result in OutOfMemory problems when the stream is too large? 2) The logic used to see that the exception must be thrown is a) Read length bytes (InputStream.read(byte[] b, int off, int len)) b) Read one more byte (read()) if this returns -1 means that the stream has no more data. If the above two steps is the algorithm for the logic the following snippet from the API doc of InputStream.read(byte[] b, int off, int len) might be relevant "An attempt is made to read as many as len bytes, but a smaller number may be read, possibly zero. The number of bytes actually read is returned as an integer." 3) Also in derby2017_try1.diff bytesToRead is no longer used. bytesToRead is set to the minimum of DssConstants.MAX_DSS_LEN - 6 - 4 - 1 - extendedLengthByteCount. I think this is the maximum chunk size that can be sent. Has derby2017_try1.diff been tried with a stream of size greater than 32767? Sorry if the above seem like random thoughts.
          Hide
          V.Narayanan added a comment -

          Some thoughts on this issue.

          The solution for this is difficult because of the following reason

          The idea of not causing an OutOfMemory error in the client is that
          you should transmit the data without materializing it.

          But how would you get the length then? This would be necessary to
          throw the stream truncation exception that is thrown.

          I somehow feel that the trick of not writing this data should lie in the server
          and the client rather than in the client alone.

          a) The server should keep with it the data it gets from the stream sent as chunks
          without writing it into the database.

          b) When the client reaches the end of the stream it comes to know that the
          truncation has happened it would inform the server that truncation has
          happened and the server does not write the data.

          The above outlined steps would be possible if the PreparedStatement were
          converted to use locators. So when a setBinaryStream is called

          a) create a locator value

          b) write the stream to the locator value(using CLOBSETSTRING or BLOBSETBYTES)

          c) If truncation happens Call CLOBRELEASELOCATOR and release this value.
          Note: here we have not gone to the PreparedStatement execute as yet.

          d) If there is not truncation do what happens in locators attach the stream
          to the PreparedStatement as happens for locators now.

          Drawbacks
          ---------

          1) This implementation would mean not using Layer-B streaming for Prepared Statements.
          Layer-B streaming is a better way to transmit this stream data than locators
          which was the reason that during locator work the decision was taken to
          retain Layer-B implementation here. See Derby-2529.

          2) Locators involve building a temporary LOB on the server. This would again
          make it less efficient than Layer-B streaming.

          Show
          V.Narayanan added a comment - Some thoughts on this issue. The solution for this is difficult because of the following reason The idea of not causing an OutOfMemory error in the client is that you should transmit the data without materializing it. But how would you get the length then? This would be necessary to throw the stream truncation exception that is thrown. I somehow feel that the trick of not writing this data should lie in the server and the client rather than in the client alone. a) The server should keep with it the data it gets from the stream sent as chunks without writing it into the database. b) When the client reaches the end of the stream it comes to know that the truncation has happened it would inform the server that truncation has happened and the server does not write the data. The above outlined steps would be possible if the PreparedStatement were converted to use locators. So when a setBinaryStream is called a) create a locator value b) write the stream to the locator value(using CLOBSETSTRING or BLOBSETBYTES) c) If truncation happens Call CLOBRELEASELOCATOR and release this value. Note: here we have not gone to the PreparedStatement execute as yet. d) If there is not truncation do what happens in locators attach the stream to the PreparedStatement as happens for locators now. Drawbacks --------- 1) This implementation would mean not using Layer-B streaming for Prepared Statements. Layer-B streaming is a better way to transmit this stream data than locators which was the reason that during locator work the decision was taken to retain Layer-B implementation here. See Derby-2529. 2) Locators involve building a temporary LOB on the server. This would again make it less efficient than Layer-B streaming.
          Hide
          Mayuresh Nirhali added a comment -

          Thanks narayanan for your comments.

          As I mentioned earlier, this patch is incomplete. I was aware of 2) and 3) from your comments, but wanted to put up a prototype for this different approach, so that it is clear; hence the patch.

          But, with 1) that I missed, I see the challenges with this approach. Materializing large streams at client side is not preferred at all.

          However, length must be known in order to construct appropriate DSS. JDBC 3.0 spec hints that the length specified in setBinaryStream should be equal to the length of the stream. Validating this is preferred at Client side to avoid the cost of data transfer in such error cases.

          Making the behavior consistent with the embedded is tricky here due to the client-server nature.

          ... looking for more inputs.

          Show
          Mayuresh Nirhali added a comment - Thanks narayanan for your comments. As I mentioned earlier, this patch is incomplete. I was aware of 2) and 3) from your comments, but wanted to put up a prototype for this different approach, so that it is clear; hence the patch. But, with 1) that I missed, I see the challenges with this approach. Materializing large streams at client side is not preferred at all. However, length must be known in order to construct appropriate DSS. JDBC 3.0 spec hints that the length specified in setBinaryStream should be equal to the length of the stream. Validating this is preferred at Client side to avoid the cost of data transfer in such error cases. Making the behavior consistent with the embedded is tricky here due to the client-server nature. ... looking for more inputs.
          Hide
          Knut Anders Hatlen added a comment -

          I think what Narayanan proposed sounds interesting. It would solve both the OutOfMemoryError problem and the problems with invalid length or errors when reading the stream, and it would make the commit/rollback behaviour identical on client and embedded.

          Of the drawbacks he mentioned, I agree that (2) probably would reduce the performance. Not sure how much, though. However, I'm not sure drawback (1) is such a big issue. If I remember correctly, Tomohito tested the performance of layer A vs layer B streaming once and didn't notice any improvement with layer B.

          Show
          Knut Anders Hatlen added a comment - I think what Narayanan proposed sounds interesting. It would solve both the OutOfMemoryError problem and the problems with invalid length or errors when reading the stream, and it would make the commit/rollback behaviour identical on client and embedded. Of the drawbacks he mentioned, I agree that (2) probably would reduce the performance. Not sure how much, though. However, I'm not sure drawback (1) is such a big issue. If I remember correctly, Tomohito tested the performance of layer A vs layer B streaming once and didn't notice any improvement with layer B.
          Hide
          Mayuresh Nirhali added a comment -

          Thanks a lot Narayanan and Knut.

          After some more investigation, I figured that the Locator support needed to implement the suggested solution is not yet complete on the trunk.

          Also, regarding the drawbacks narayanan mentioned, I feel drawback 2) could degrade the performance considerably because of more number of round-trips even before the execution.

          I think, It make sense to revisit this issue later (probably after 10.3).
          Comments are welcome!

          Show
          Mayuresh Nirhali added a comment - Thanks a lot Narayanan and Knut. After some more investigation, I figured that the Locator support needed to implement the suggested solution is not yet complete on the trunk. Also, regarding the drawbacks narayanan mentioned, I feel drawback 2) could degrade the performance considerably because of more number of round-trips even before the execution. I think, It make sense to revisit this issue later (probably after 10.3). Comments are welcome!
          Hide
          Kathey Marsden added a comment -

          Mayuresh, are you actively working on this issue?

          Show
          Kathey Marsden added a comment - Mayuresh, are you actively working on this issue?
          Hide
          Mayuresh Nirhali added a comment -

          Cathey, I am not working on this issue.
          Currently, I am tied up with something and hence un-assigning myself.

          Show
          Mayuresh Nirhali added a comment - Cathey, I am not working on this issue. Currently, I am tied up with something and hence un-assigning myself.
          Hide
          V.Narayanan added a comment -

          One of the solutions proposed in the discussions above is converting Prepared/Callable
          statements to use locators. Has this solution been rejected?

          Show
          V.Narayanan added a comment - One of the solutions proposed in the discussions above is converting Prepared/Callable statements to use locators. Has this solution been rejected?
          Hide
          Knut Anders Hatlen added a comment -

          No, it hasn't been rejected, but it was indicated that it might have negative impact on the performance. One alternative is to add a product-specific code point so that we have a way to indicate to the server that an error has happened. See "Developing Product-Unique Extensions" in DRDA, Version 4, Volume 3, Section 2.5.5.4 for details.

          Show
          Knut Anders Hatlen added a comment - No, it hasn't been rejected, but it was indicated that it might have negative impact on the performance. One alternative is to add a product-specific code point so that we have a way to indicate to the server that an error has happened. See "Developing Product-Unique Extensions" in DRDA, Version 4, Volume 3, Section 2.5.5.4 for details.
          Hide
          Knut Anders Hatlen added a comment -

          Triaged for 10.5.2.

          Show
          Knut Anders Hatlen added a comment - Triaged for 10.5.2.
          Hide
          Kristian Waagan added a comment -

          (cleared the in-progress flag, had to assign the issue to myself to do this)

          Show
          Kristian Waagan added a comment - (cleared the in-progress flag, had to assign the issue to myself to do this)
          Hide
          Kristian Waagan added a comment -

          I haven't looked into the details, but DRDA has a mechanism for interrupting DRDA requests. However, this requires that the client opens a new connection to the server to issue the request.
          Without knowing how hard this will be to implement, does this sound as a viable solution?

          Interrupting the server is an exceptional case, so I guess the performance of the mentioned solution isn't critical (i.e. create new connection, send the request, receive reply, throw exception and do cleanup on the client side will take a while compared to sending something on the existing connection). I suppose we can use the same mechanism if we need to be able to interrupt the server in other states.

          For those interested, see DRDA vol 1 - 4.4.14 Interrupting a Running DRDA Request.
          As I said, I haven't yet studied the spec, nor Derby's DRDA implementation, so I can't say for sure this will work.

          Show
          Kristian Waagan added a comment - I haven't looked into the details, but DRDA has a mechanism for interrupting DRDA requests. However, this requires that the client opens a new connection to the server to issue the request. Without knowing how hard this will be to implement, does this sound as a viable solution? Interrupting the server is an exceptional case, so I guess the performance of the mentioned solution isn't critical (i.e. create new connection, send the request, receive reply, throw exception and do cleanup on the client side will take a while compared to sending something on the existing connection). I suppose we can use the same mechanism if we need to be able to interrupt the server in other states. For those interested, see DRDA vol 1 - 4.4.14 Interrupting a Running DRDA Request. As I said, I haven't yet studied the spec, nor Derby's DRDA implementation, so I can't say for sure this will work.
          Hide
          Rick Hillegas added a comment -

          Hi Kristian,

          I may not be understanding the solution you are proposing. However, I wonder if it might not be air-tight. It seems to me that there might be a race condition. Wrong results might still be committed if the statement manages to complete before the client can open a second connection and interrupt the server.

          Show
          Rick Hillegas added a comment - Hi Kristian, I may not be understanding the solution you are proposing. However, I wonder if it might not be air-tight. It seems to me that there might be a race condition. Wrong results might still be committed if the statement manages to complete before the client can open a second connection and interrupt the server.
          Hide
          Kristian Waagan added a comment -

          Hi Rick,

          I agree that in the general case there might be race conditions, but that shouldn't be a problem in this specific case. To cancel truly running requests I understand that a second connection is needed, since the existing one is stuck waiting for the server reply.
          I think we can control when the statement completes, since we are controlling when we send data to the server. Be holding back data, the server thread will be stuck.

          However, when prototyping this, it seems to me that the complexity of this approach is a lot higher than adding a product specific code point.
          We would need to handle the following issues:

          • creating a new connection based on an existing connection. Here we would need to have things like the user name and password available.
          • introducing an interrupt token (sent to the client on ACCRDB, sent back to the server to identify which session to interrupt)
          • looking up session based on interrupt token on the server side
          • interrupt logic on the server side

          We might get away with one thread on the client side, but on the server side we need two threads / connections to communicate with each other. The overall process would be something like this (there are several variations):

          • client sends data
          • client detects problem (too much data or too little data)
          • client creates a second connection (the server session is currently stuck in a read loop awaiting more data, or maybe the last piece of data)
          • the second thread on the server (new connection) signals that a DRDA interrupt has been requested
          • the client sends valid data (wrt the DRDA protocol) to the server session to wake it up
          • the server reads data, detects interrupt request and aborts the current statement
          • the second thread on the server sends a reply to the interrupt request itself
          • the client reads the reply (second connection)
          • the first server thread informs that the running request was interrupted and canceled
          • the client reads reply from the original connection and throws exception

          In addition to the code complexity, we must also make sure we obey the DRDA protocol rules to avoid network protocol errors.

          As a side note, the code in org.apache.derby.impl.drda.DRDAConnThread.convertAsByteArrayInputStream can cause OOME on the server for if more than one large LOB is transferred from the client in the same statement.

          Show
          Kristian Waagan added a comment - Hi Rick, I agree that in the general case there might be race conditions, but that shouldn't be a problem in this specific case. To cancel truly running requests I understand that a second connection is needed, since the existing one is stuck waiting for the server reply. I think we can control when the statement completes, since we are controlling when we send data to the server. Be holding back data, the server thread will be stuck. However, when prototyping this, it seems to me that the complexity of this approach is a lot higher than adding a product specific code point. We would need to handle the following issues: creating a new connection based on an existing connection. Here we would need to have things like the user name and password available. introducing an interrupt token (sent to the client on ACCRDB, sent back to the server to identify which session to interrupt) looking up session based on interrupt token on the server side interrupt logic on the server side We might get away with one thread on the client side, but on the server side we need two threads / connections to communicate with each other. The overall process would be something like this (there are several variations): client sends data client detects problem (too much data or too little data) client creates a second connection (the server session is currently stuck in a read loop awaiting more data, or maybe the last piece of data) the second thread on the server (new connection) signals that a DRDA interrupt has been requested the client sends valid data (wrt the DRDA protocol) to the server session to wake it up the server reads data, detects interrupt request and aborts the current statement the second thread on the server sends a reply to the interrupt request itself the client reads the reply (second connection) the first server thread informs that the running request was interrupted and canceled the client reads reply from the original connection and throws exception In addition to the code complexity, we must also make sure we obey the DRDA protocol rules to avoid network protocol errors. As a side note, the code in org.apache.derby.impl.drda.DRDAConnThread.convertAsByteArrayInputStream can cause OOME on the server for if more than one large LOB is transferred from the client in the same statement.
          Hide
          Kristian Waagan added a comment -

          Attached a first revision of a regression test as patch 2a.

          Show
          Kristian Waagan added a comment - Attached a first revision of a regression test as patch 2a.
          Hide
          Kristian Waagan added a comment -

          I think I may have a feasible solution for this bug. I have investigated a few different approaches, and I will soon post some patches and approach descriptions.

          Show
          Kristian Waagan added a comment - I think I may have a feasible solution for this bug. I have investigated a few different approaches, and I will soon post some patches and approach descriptions.
          Hide
          Kristian Waagan added a comment -

          Attaching a preview patch, where a Derby specific trailing status flag is sent as the last byte. The status flag is appended to the user data.
          I think this solution is the simplest approach, and it should also be easy to deal with the compatibility issue. The status flag is stripped away, it isn't saved to disk.

          The idea is to make the statement fail on the server side, and let the embedded driver deal with the clean-up. The error will then be reported to the client driver as any other error that may occur during statement execution.
          In the case of multiple stream sources and an error in the first one, I figured it is easier to continue with the statement execution rather than to abort early on. This is because one would otherwise need recovery logic in both the client driver and the network server. As a possible optimization I thought of adding a status called STREAM_SKIPPED. This, possibly together with a single "fake" data byte, will be sent to the server for any remaining data sources. Doing this avoids having to read and send potentially large values when we know that the statement will fail.

          The various statuses are modeled after the current code in the client code. We don't need all of them (we really only need SUCCESS and FAILURE), but maybe they can be helpful for debugging? Having many status values doesn't add overhead to the network protocol (we always send one byte).
          Here's the current list:
          STREAM_OK
          STREAM_UNKNOWN_LENGTH (used with layer-B streaming and in an assert)
          STREAM_READ_ERROR
          STREAM_READ_ERROR_ON_LEN_VAL
          STREAM_TOO_SHORT
          STREAM_TOO_LONG
          STREAM_SKIPPED

          I'm considering removing at least STREAM_UNKNOWN_LENGTH and STREAM_READ_ERROR_ON_LEN_VAL (effectively merging them with STREAM_OK and STREAM_READ_ERROR, respectively).

          Remaining work on the patch:
          o layer-B streaming code
          o encryption code [1]
          o error handling on the client (I don't think we need to use the accumulated error mechanism when sending the status byte)

          I'll continue work on finalizing the patch, but feel free to ask questions and comment on the proposed approach.

          [1] We are currently materializing the value here, and I won't improve this as part of this issue.

          Show
          Kristian Waagan added a comment - Attaching a preview patch, where a Derby specific trailing status flag is sent as the last byte. The status flag is appended to the user data. I think this solution is the simplest approach, and it should also be easy to deal with the compatibility issue. The status flag is stripped away, it isn't saved to disk. The idea is to make the statement fail on the server side, and let the embedded driver deal with the clean-up. The error will then be reported to the client driver as any other error that may occur during statement execution. In the case of multiple stream sources and an error in the first one, I figured it is easier to continue with the statement execution rather than to abort early on. This is because one would otherwise need recovery logic in both the client driver and the network server. As a possible optimization I thought of adding a status called STREAM_SKIPPED. This, possibly together with a single "fake" data byte, will be sent to the server for any remaining data sources. Doing this avoids having to read and send potentially large values when we know that the statement will fail. The various statuses are modeled after the current code in the client code. We don't need all of them (we really only need SUCCESS and FAILURE), but maybe they can be helpful for debugging? Having many status values doesn't add overhead to the network protocol (we always send one byte). Here's the current list: STREAM_OK STREAM_UNKNOWN_LENGTH (used with layer-B streaming and in an assert) STREAM_READ_ERROR STREAM_READ_ERROR_ON_LEN_VAL STREAM_TOO_SHORT STREAM_TOO_LONG STREAM_SKIPPED I'm considering removing at least STREAM_UNKNOWN_LENGTH and STREAM_READ_ERROR_ON_LEN_VAL (effectively merging them with STREAM_OK and STREAM_READ_ERROR, respectively). Remaining work on the patch: o layer-B streaming code o encryption code [1] o error handling on the client (I don't think we need to use the accumulated error mechanism when sending the status byte) I'll continue work on finalizing the patch, but feel free to ask questions and comment on the proposed approach. [1] We are currently materializing the value here, and I won't improve this as part of this issue.
          Hide
          Knut Anders Hatlen added a comment -

          Hi Kristian,

          This sounds like a good approach to me. Also, +1 to keeping the code simple and not optimizing for possible failure scenarios. Exhausting all streams, even if the first one fails, sounds perfectly acceptable.

          One question: If the user stream fails with an exception on the client side, will the original exception be the one that's reported to the user, or will it be the synthetic exception produced on the server?

          Show
          Knut Anders Hatlen added a comment - Hi Kristian, This sounds like a good approach to me. Also, +1 to keeping the code simple and not optimizing for possible failure scenarios. Exhausting all streams, even if the first one fails, sounds perfectly acceptable. One question: If the user stream fails with an exception on the client side, will the original exception be the one that's reported to the user, or will it be the synthetic exception produced on the server?
          Hide
          Kristian Waagan added a comment -

          Thanks for having a look, Knut Anders.

          Knut Anders wrote:


          One question: If the user stream fails with an exception on the client side, will the original exception be the one that's reported to the user, or will it be the synthetic exception produced on the server?


          With the current code (preview with some changes), where nothing has been done to the exception handling, the following exceptions are produced for the embedded and the client driver:

          ---------> Client

          ----- SQLException -----
          SQLState: XN015
          Error Code: 20000
          Message: Network protocol error: the specified size of the InputStream, parameter #1, is less than the actual InputStream length.
          java.sql.SQLException: Network protocol error: the specified size of the InputStream, parameter #1, is less than the actual InputStream length.
          at org.apache.derby.client.am.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:96)
          at org.apache.derby.client.am.SqlException.getSQLException(SqlException.java:358)
          at org.apache.derby.client.am.PreparedStatement.executeUpdate(PreparedStatement.java:399)
          at org.apache.derbyTesting.functionTests.tests.jdbcapi.StreamErrRepro.test(StreamErrRepro.java:38)
          at org.apache.derbyTesting.functionTests.tests.jdbcapi.StreamErrRepro.main(StreamErrRepro.java:21)
          Caused by: org.apache.derby.client.am.SqlException: Network protocol error: the specified size of the InputStream, parameter #1, is less than the actual InputStream length.
          at org.apache.derby.client.net.Request.writePlainScalarStream(Request.java:540)
          at org.apache.derby.client.net.Request.writeScalarStream(Request.java:264)
          at org.apache.derby.client.net.Request.writeScalarStream(Request.java:679)
          at org.apache.derby.client.net.NetStatementRequest.buildEXTDTA(NetStatementRequest.java:1011)
          at org.apache.derby.client.net.NetStatementRequest.writeExecute(NetStatementRequest.java:147)
          at org.apache.derby.client.net.NetPreparedStatement.writeExecute_(NetPreparedStatement.java:178)
          at org.apache.derby.client.am.PreparedStatement.writeExecute(PreparedStatement.java:1855)
          at org.apache.derby.client.am.PreparedStatement.flowExecute(PreparedStatement.java:2085)
          at org.apache.derby.client.am.PreparedStatement.executeUpdateX(PreparedStatement.java:404)
          at org.apache.derby.client.am.PreparedStatement.executeUpdate(PreparedStatement.java:390)
          ... 2 more

          ----- SQLException -----
          SQLState: XCL30
          Error Code: -1
          Message: An IOException was thrown when reading a 'java.lang.String' from an InputStream.
          java.sql.SQLException: An IOException was thrown when reading a 'java.lang.String' from an InputStream.
          at org.apache.derby.client.am.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:96)
          at org.apache.derby.client.am.SqlException.getSQLException(SqlException.java:358)
          at org.apache.derby.client.am.SqlException.getSQLException(SqlException.java:367)
          at org.apache.derby.client.am.PreparedStatement.executeUpdate(PreparedStatement.java:399)
          at org.apache.derbyTesting.functionTests.tests.jdbcapi.StreamErrRepro.test(StreamErrRepro.java:38)
          at org.apache.derbyTesting.functionTests.tests.jdbcapi.StreamErrRepro.main(StreamErrRepro.java:21)
          Caused by: org.apache.derby.client.am.SqlException: An IOException was thrown when reading a 'java.lang.String' from an InputStream.
          at org.apache.derby.client.am.Statement.completeExecute(Statement.java:1601)
          at org.apache.derby.client.net.NetStatementReply.parseEXCSQLSTTreply(NetStatementReply.java:322)
          at org.apache.derby.client.net.NetStatementReply.readExecute(NetStatementReply.java:71)
          at org.apache.derby.client.net.StatementReply.readExecute(StatementReply.java:55)
          at org.apache.derby.client.net.NetPreparedStatement.readExecute_(NetPreparedStatement.java:189)
          at org.apache.derby.client.am.PreparedStatement.readExecute(PreparedStatement.java:1865)
          at org.apache.derby.client.am.PreparedStatement.flowExecute(PreparedStatement.java:2162)
          at org.apache.derby.client.am.PreparedStatement.executeUpdateX(PreparedStatement.java:404)
          at org.apache.derby.client.am.PreparedStatement.executeUpdate(PreparedStatement.java:390)
          ... 2 more
          Caused by: org.apache.derby.client.am.SqlException: Java exception: 'Input stream did not have exact amount of data as the requested length.: org.apache.derby.iapi.services.io.DerbyIOException'.
          ... 11 more

          ----- SQLException -----
          SQLState: XJ001
          Error Code: 99999
          Message: Java exception: 'Input stream did not have exact amount of data as the requested length.: org.apache.derby.iapi.services.io.DerbyIOException'.
          java.sql.SQLNonTransientConnectionException: Java exception: 'Input stream did not have exact amount of data as the requested length.: org.apache.derby.iapi.services.io.DerbyIOException'.
          at org.apache.derby.client.am.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:70)
          at org.apache.derby.client.am.SqlException.getSQLException(SqlException.java:358)
          at org.apache.derby.client.am.SqlException.getSQLException(SqlException.java:367)
          at org.apache.derby.client.am.SqlException.getSQLException(SqlException.java:367)
          at org.apache.derby.client.am.PreparedStatement.executeUpdate(PreparedStatement.java:399)
          at org.apache.derbyTesting.functionTests.tests.jdbcapi.StreamErrRepro.test(StreamErrRepro.java:38)
          at org.apache.derbyTesting.functionTests.tests.jdbcapi.StreamErrRepro.main(StreamErrRepro.java:21)
          Caused by: org.apache.derby.client.am.SqlException: Java exception: 'Input stream did not have exact amount of data as the requested length.: org.apache.derby.iapi.services.io.DerbyIOException'.
          at org.apache.derby.client.am.Statement.completeExecute(Statement.java:1601)
          at org.apache.derby.client.net.NetStatementReply.parseEXCSQLSTTreply(NetStatementReply.java:322)
          at org.apache.derby.client.net.NetStatementReply.readExecute(NetStatementReply.java:71)
          at org.apache.derby.client.net.StatementReply.readExecute(StatementReply.java:55)
          at org.apache.derby.client.net.NetPreparedStatement.readExecute_(NetPreparedStatement.java:189)
          at org.apache.derby.client.am.PreparedStatement.readExecute(PreparedStatement.java:1865)
          at org.apache.derby.client.am.PreparedStatement.flowExecute(PreparedStatement.java:2162)
          at org.apache.derby.client.am.PreparedStatement.executeUpdateX(PreparedStatement.java:404)
          at org.apache.derby.client.am.PreparedStatement.executeUpdate(PreparedStatement.java:390)
          ... 2 more

          ---------> Embedded

          java.sql.SQLException: An IOException was thrown when reading a 'java.lang.String' from an InputStream.

          ----- SQLException -----
          SQLState: XCL30
          Error Code: 20000
          Message: An IOException was thrown when reading a 'java.lang.String' from an InputStream.
          java.sql.SQLException: An IOException was thrown when reading a 'java.lang.String' from an InputStream.
          at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:95)
          at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Util.java:142)
          at org.apache.derby.impl.jdbc.Util.seeNextException(Util.java:278)
          at org.apache.derby.impl.jdbc.TransactionResourceImpl.wrapInSQLException(TransactionResourceImpl.java:398)
          at org.apache.derby.impl.jdbc.TransactionResourceImpl.handleException(TransactionResourceImpl.java:346)
          at org.apache.derby.impl.jdbc.EmbedConnection.handleException(EmbedConnection.java:2269)
          at org.apache.derby.impl.jdbc.ConnectionChild.handleException(ConnectionChild.java:81)
          at org.apache.derby.impl.jdbc.EmbedStatement.executeStatement(EmbedStatement.java:1321)
          at org.apache.derby.impl.jdbc.EmbedPreparedStatement.executeStatement(EmbedPreparedStatement.java:1673)
          at org.apache.derby.impl.jdbc.EmbedPreparedStatement.executeUpdate(EmbedPreparedStatement.java:303)
          at org.apache.derbyTesting.functionTests.tests.jdbcapi.StreamErrRepro.test(StreamErrRepro.java:38)
          at org.apache.derbyTesting.functionTests.tests.jdbcapi.StreamErrRepro.main(StreamErrRepro.java:22)
          Caused by: java.sql.SQLException: An IOException was thrown when reading a 'java.lang.String' from an InputStream.
          at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(SQLExceptionFactory.java:45)
          at org.apache.derby.impl.jdbc.SQLExceptionFactory40.wrapArgsForTransportAcrossDRDA(SQLExceptionFactory40.java:119)
          at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:70)
          ... 11 more
          Caused by: java.sql.SQLException: Java exception: 'Input stream did not have exact amount of data as the requested length.: org.apache.derby.iapi.services.io.DerbyIOException'.
          at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(SQLExceptionFactory.java:45)
          at org.apache.derby.impl.jdbc.SQLExceptionFactory40.wrapArgsForTransportAcrossDRDA(SQLExceptionFactory40.java:119)
          at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:70)
          at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Util.java:142)
          at org.apache.derby.impl.jdbc.Util.javaException(Util.java:299)
          at org.apache.derby.impl.jdbc.TransactionResourceImpl.wrapInSQLException(TransactionResourceImpl.java:403)
          ... 9 more
          Caused by: org.apache.derby.iapi.services.io.DerbyIOException: Input stream did not have exact amount of data as the requested length.
          at org.apache.derby.iapi.types.ReaderToUTF8Stream.checkSufficientData(ReaderToUTF8Stream.java:420)
          at org.apache.derby.iapi.types.ReaderToUTF8Stream.fillBuffer(ReaderToUTF8Stream.java:378)
          at org.apache.derby.iapi.types.ReaderToUTF8Stream.read(ReaderToUTF8Stream.java:197)
          at java.io.DataInputStream.readUnsignedShort(Unknown Source)
          at org.apache.derby.iapi.types.SQLChar.readExternal(SQLChar.java:1050)
          at org.apache.derby.iapi.types.SQLChar.getString(SQLChar.java:695)
          at org.apache.derby.iapi.types.SQLVarchar.normalize(SQLVarchar.java:148)
          at org.apache.derby.iapi.types.DataTypeDescriptor.normalize(DataTypeDescriptor.java:648)
          at org.apache.derby.impl.sql.execute.NormalizeResultSet.normalizeColumn(NormalizeResultSet.java:329)
          at org.apache.derby.impl.sql.execute.NormalizeResultSet.normalizeRow(NormalizeResultSet.java:373)
          at org.apache.derby.impl.sql.execute.NormalizeResultSet.getNextRowCore(NormalizeResultSet.java:188)
          at org.apache.derby.impl.sql.execute.DMLWriteResultSet.getNextRowCore(DMLWriteResultSet.java:127)
          at org.apache.derby.impl.sql.execute.InsertResultSet.open(InsertResultSet.java:494)
          at org.apache.derby.impl.sql.GenericPreparedStatement.executeStmt(GenericPreparedStatement.java:436)
          at org.apache.derby.impl.sql.GenericPreparedStatement.execute(GenericPreparedStatement.java:317)
          at org.apache.derby.impl.jdbc.EmbedStatement.executeStatement(EmbedStatement.java:1232)
          ... 4 more

          ----- SQLException -----
          SQLState: XJ001
          Error Code: 0
          Message: Java exception: 'Input stream did not have exact amount of data as the requested length.: org.apache.derby.iapi.services.io.DerbyIOException'.
          java.sql.SQLException: Java exception: 'Input stream did not have exact amount of data as the requested length.: org.apache.derby.iapi.services.io.DerbyIOException'.
          at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:95)
          at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Util.java:142)
          at org.apache.derby.impl.jdbc.Util.javaException(Util.java:299)
          at org.apache.derby.impl.jdbc.TransactionResourceImpl.wrapInSQLException(TransactionResourceImpl.java:403)
          at org.apache.derby.impl.jdbc.TransactionResourceImpl.wrapInSQLException(TransactionResourceImpl.java:398)
          at org.apache.derby.impl.jdbc.TransactionResourceImpl.handleException(TransactionResourceImpl.java:346)
          at org.apache.derby.impl.jdbc.EmbedConnection.handleException(EmbedConnection.java:2269)
          at org.apache.derby.impl.jdbc.ConnectionChild.handleException(ConnectionChild.java:81)
          at org.apache.derby.impl.jdbc.EmbedStatement.executeStatement(EmbedStatement.java:1321)
          at org.apache.derby.impl.jdbc.EmbedPreparedStatement.executeStatement(EmbedPreparedStatement.java:1673)
          at org.apache.derby.impl.jdbc.EmbedPreparedStatement.executeUpdate(EmbedPreparedStatement.java:303)
          at org.apache.derbyTesting.functionTests.tests.jdbcapi.StreamErrRepro.test(StreamErrRepro.java:38)
          at org.apache.derbyTesting.functionTests.tests.jdbcapi.StreamErrRepro.main(StreamErrRepro.java:22)
          Caused by: java.sql.SQLException: Java exception: 'Input stream did not have exact amount of data as the requested length.: org.apache.derby.iapi.services.io.DerbyIOException'.
          at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(SQLExceptionFactory.java:45)
          at org.apache.derby.impl.jdbc.SQLExceptionFactory40.wrapArgsForTransportAcrossDRDA(SQLExceptionFactory40.java:119)
          at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:70)
          ... 12 more
          Caused by: org.apache.derby.iapi.services.io.DerbyIOException: Input stream did not have exact amount of data as the requested length.
          at org.apache.derby.iapi.types.ReaderToUTF8Stream.checkSufficientData(ReaderToUTF8Stream.java:420)
          at org.apache.derby.iapi.types.ReaderToUTF8Stream.fillBuffer(ReaderToUTF8Stream.java:378)
          at org.apache.derby.iapi.types.ReaderToUTF8Stream.read(ReaderToUTF8Stream.java:197)
          at java.io.DataInputStream.readUnsignedShort(Unknown Source)
          at org.apache.derby.iapi.types.SQLChar.readExternal(SQLChar.java:1050)
          at org.apache.derby.iapi.types.SQLChar.getString(SQLChar.java:695)
          at org.apache.derby.iapi.types.SQLVarchar.normalize(SQLVarchar.java:148)
          at org.apache.derby.iapi.types.DataTypeDescriptor.normalize(DataTypeDescriptor.java:648)
          at org.apache.derby.impl.sql.execute.NormalizeResultSet.normalizeColumn(NormalizeResultSet.java:329)
          at org.apache.derby.impl.sql.execute.NormalizeResultSet.normalizeRow(NormalizeResultSet.java:373)
          at org.apache.derby.impl.sql.execute.NormalizeResultSet.getNextRowCore(NormalizeResultSet.java:188)
          at org.apache.derby.impl.sql.execute.DMLWriteResultSet.getNextRowCore(DMLWriteResultSet.java:127)
          at org.apache.derby.impl.sql.execute.InsertResultSet.open(InsertResultSet.java:494)
          at org.apache.derby.impl.sql.GenericPreparedStatement.executeStmt(GenericPreparedStatement.java:436)
          at org.apache.derby.impl.sql.GenericPreparedStatement.execute(GenericPreparedStatement.java:317)
          at org.apache.derby.impl.jdbc.EmbedStatement.executeStatement(EmbedStatement.java:1232)
          ... 4 more

          When using the client driver, the following stack trace is written to derby.log:
          ============= begin nested exception, level (1) ===========
          org.apache.derby.iapi.services.io.DerbyIOException: Input stream did not have exact amount of data as the requested length.
          at org.apache.derby.impl.drda.EXTDTAReaderInputStream.checkStatus(EXTDTAReaderInputStream.java:173)
          at org.apache.derby.impl.drda.StandardEXTDTAReaderInputStream.read(StandardEXTDTAReaderInputStream.java:146)
          at sun.nio.cs.StreamDecoder.readBytes(Unknown Source)
          at sun.nio.cs.StreamDecoder.implRead(Unknown Source)
          at sun.nio.cs.StreamDecoder.read(Unknown Source)
          at sun.nio.cs.StreamDecoder.read0(Unknown Source)
          at sun.nio.cs.StreamDecoder.read(Unknown Source)
          at java.io.InputStreamReader.read(Unknown Source)
          at org.apache.derby.iapi.services.io.LimitReader.read(LimitReader.java:57)
          at org.apache.derby.iapi.types.ReaderToUTF8Stream.fillBuffer(ReaderToUTF8Stream.java:352)
          at org.apache.derby.iapi.types.ReaderToUTF8Stream.read(ReaderToUTF8Stream.java:197)
          at java.io.DataInputStream.readUnsignedShort(Unknown Source)
          at org.apache.derby.iapi.types.SQLChar.readExternal(SQLChar.java:1050)
          at org.apache.derby.iapi.types.SQLChar.getString(SQLChar.java:695)
          at org.apache.derby.iapi.types.SQLVarchar.normalize(SQLVarchar.java:148)
          at org.apache.derby.iapi.types.DataTypeDescriptor.normalize(DataTypeDescriptor.java:648)
          at org.apache.derby.impl.sql.execute.NormalizeResultSet.normalizeColumn(NormalizeResultSet.java:329)
          at org.apache.derby.impl.sql.execute.NormalizeResultSet.normalizeRow(NormalizeResultSet.java:373)
          at org.apache.derby.impl.sql.execute.NormalizeResultSet.getNextRowCore(NormalizeResultSet.java:188)
          at org.apache.derby.impl.sql.execute.DMLWriteResultSet.getNextRowCore(DMLWriteResultSet.java:127)
          at org.apache.derby.impl.sql.execute.InsertResultSet.open(InsertResultSet.java:494)
          at org.apache.derby.impl.sql.GenericPreparedStatement.executeStmt(GenericPreparedStatement.java:436)
          at org.apache.derby.impl.sql.GenericPreparedStatement.execute(GenericPreparedStatement.java:317)
          at org.apache.derby.impl.jdbc.EmbedStatement.executeStatement(EmbedStatement.java:1232)
          at org.apache.derby.impl.jdbc.EmbedPreparedStatement.executeStatement(EmbedPreparedStatement.java:1673)
          at org.apache.derby.impl.jdbc.EmbedPreparedStatement.execute(EmbedPreparedStatement.java:1328)
          at org.apache.derby.impl.drda.DRDAStatement.execute(DRDAStatement.java:672)
          at org.apache.derby.impl.drda.DRDAConnThread.parseEXCSQLSTTobjects(DRDAConnThread.java:4262)
          at org.apache.derby.impl.drda.DRDAConnThread.parseEXCSQLSTT(DRDAConnThread.java:4085)
          at org.apache.derby.impl.drda.DRDAConnThread.processCommands(DRDAConnThread.java:1004)
          at org.apache.derby.impl.drda.DRDAConnThread.run(DRDAConnThread.java:291)
          ============= end nested exception, level (1) ===========

          If we disable the use of the accumulated exception mechanism on the client (when sending the status byte, i.e. server and client at 10.6+), we get the same SQL states (XCL30 and XJ001) in both drivers. Looking at the traces, I do see DerbyIOException. This was introduced to make Derby able to rewrite IOException to SQLException with the correct state, but this is obviously not done for this code path. For instance, we may want to differentiate between a generic read error from a user stream and an error thrown by Derby due to a mismatch between the specified and the actual stream length.

          I would prefer to throw a different exception than the generic XJ001, but I think this can be handled under a different Jira. For instance, XJ023 may be better suited.

          Show
          Kristian Waagan added a comment - Thanks for having a look, Knut Anders. Knut Anders wrote: One question: If the user stream fails with an exception on the client side, will the original exception be the one that's reported to the user, or will it be the synthetic exception produced on the server? With the current code (preview with some changes), where nothing has been done to the exception handling, the following exceptions are produced for the embedded and the client driver: ---------> Client ----- SQLException ----- SQLState: XN015 Error Code: 20000 Message: Network protocol error: the specified size of the InputStream, parameter #1, is less than the actual InputStream length. java.sql.SQLException: Network protocol error: the specified size of the InputStream, parameter #1, is less than the actual InputStream length. at org.apache.derby.client.am.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:96) at org.apache.derby.client.am.SqlException.getSQLException(SqlException.java:358) at org.apache.derby.client.am.PreparedStatement.executeUpdate(PreparedStatement.java:399) at org.apache.derbyTesting.functionTests.tests.jdbcapi.StreamErrRepro.test(StreamErrRepro.java:38) at org.apache.derbyTesting.functionTests.tests.jdbcapi.StreamErrRepro.main(StreamErrRepro.java:21) Caused by: org.apache.derby.client.am.SqlException: Network protocol error: the specified size of the InputStream, parameter #1, is less than the actual InputStream length. at org.apache.derby.client.net.Request.writePlainScalarStream(Request.java:540) at org.apache.derby.client.net.Request.writeScalarStream(Request.java:264) at org.apache.derby.client.net.Request.writeScalarStream(Request.java:679) at org.apache.derby.client.net.NetStatementRequest.buildEXTDTA(NetStatementRequest.java:1011) at org.apache.derby.client.net.NetStatementRequest.writeExecute(NetStatementRequest.java:147) at org.apache.derby.client.net.NetPreparedStatement.writeExecute_(NetPreparedStatement.java:178) at org.apache.derby.client.am.PreparedStatement.writeExecute(PreparedStatement.java:1855) at org.apache.derby.client.am.PreparedStatement.flowExecute(PreparedStatement.java:2085) at org.apache.derby.client.am.PreparedStatement.executeUpdateX(PreparedStatement.java:404) at org.apache.derby.client.am.PreparedStatement.executeUpdate(PreparedStatement.java:390) ... 2 more ----- SQLException ----- SQLState: XCL30 Error Code: -1 Message: An IOException was thrown when reading a 'java.lang.String' from an InputStream. java.sql.SQLException: An IOException was thrown when reading a 'java.lang.String' from an InputStream. at org.apache.derby.client.am.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:96) at org.apache.derby.client.am.SqlException.getSQLException(SqlException.java:358) at org.apache.derby.client.am.SqlException.getSQLException(SqlException.java:367) at org.apache.derby.client.am.PreparedStatement.executeUpdate(PreparedStatement.java:399) at org.apache.derbyTesting.functionTests.tests.jdbcapi.StreamErrRepro.test(StreamErrRepro.java:38) at org.apache.derbyTesting.functionTests.tests.jdbcapi.StreamErrRepro.main(StreamErrRepro.java:21) Caused by: org.apache.derby.client.am.SqlException: An IOException was thrown when reading a 'java.lang.String' from an InputStream. at org.apache.derby.client.am.Statement.completeExecute(Statement.java:1601) at org.apache.derby.client.net.NetStatementReply.parseEXCSQLSTTreply(NetStatementReply.java:322) at org.apache.derby.client.net.NetStatementReply.readExecute(NetStatementReply.java:71) at org.apache.derby.client.net.StatementReply.readExecute(StatementReply.java:55) at org.apache.derby.client.net.NetPreparedStatement.readExecute_(NetPreparedStatement.java:189) at org.apache.derby.client.am.PreparedStatement.readExecute(PreparedStatement.java:1865) at org.apache.derby.client.am.PreparedStatement.flowExecute(PreparedStatement.java:2162) at org.apache.derby.client.am.PreparedStatement.executeUpdateX(PreparedStatement.java:404) at org.apache.derby.client.am.PreparedStatement.executeUpdate(PreparedStatement.java:390) ... 2 more Caused by: org.apache.derby.client.am.SqlException: Java exception: 'Input stream did not have exact amount of data as the requested length.: org.apache.derby.iapi.services.io.DerbyIOException'. ... 11 more ----- SQLException ----- SQLState: XJ001 Error Code: 99999 Message: Java exception: 'Input stream did not have exact amount of data as the requested length.: org.apache.derby.iapi.services.io.DerbyIOException'. java.sql.SQLNonTransientConnectionException: Java exception: 'Input stream did not have exact amount of data as the requested length.: org.apache.derby.iapi.services.io.DerbyIOException'. at org.apache.derby.client.am.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:70) at org.apache.derby.client.am.SqlException.getSQLException(SqlException.java:358) at org.apache.derby.client.am.SqlException.getSQLException(SqlException.java:367) at org.apache.derby.client.am.SqlException.getSQLException(SqlException.java:367) at org.apache.derby.client.am.PreparedStatement.executeUpdate(PreparedStatement.java:399) at org.apache.derbyTesting.functionTests.tests.jdbcapi.StreamErrRepro.test(StreamErrRepro.java:38) at org.apache.derbyTesting.functionTests.tests.jdbcapi.StreamErrRepro.main(StreamErrRepro.java:21) Caused by: org.apache.derby.client.am.SqlException: Java exception: 'Input stream did not have exact amount of data as the requested length.: org.apache.derby.iapi.services.io.DerbyIOException'. at org.apache.derby.client.am.Statement.completeExecute(Statement.java:1601) at org.apache.derby.client.net.NetStatementReply.parseEXCSQLSTTreply(NetStatementReply.java:322) at org.apache.derby.client.net.NetStatementReply.readExecute(NetStatementReply.java:71) at org.apache.derby.client.net.StatementReply.readExecute(StatementReply.java:55) at org.apache.derby.client.net.NetPreparedStatement.readExecute_(NetPreparedStatement.java:189) at org.apache.derby.client.am.PreparedStatement.readExecute(PreparedStatement.java:1865) at org.apache.derby.client.am.PreparedStatement.flowExecute(PreparedStatement.java:2162) at org.apache.derby.client.am.PreparedStatement.executeUpdateX(PreparedStatement.java:404) at org.apache.derby.client.am.PreparedStatement.executeUpdate(PreparedStatement.java:390) ... 2 more ---------> Embedded java.sql.SQLException: An IOException was thrown when reading a 'java.lang.String' from an InputStream. ----- SQLException ----- SQLState: XCL30 Error Code: 20000 Message: An IOException was thrown when reading a 'java.lang.String' from an InputStream. java.sql.SQLException: An IOException was thrown when reading a 'java.lang.String' from an InputStream. at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:95) at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Util.java:142) at org.apache.derby.impl.jdbc.Util.seeNextException(Util.java:278) at org.apache.derby.impl.jdbc.TransactionResourceImpl.wrapInSQLException(TransactionResourceImpl.java:398) at org.apache.derby.impl.jdbc.TransactionResourceImpl.handleException(TransactionResourceImpl.java:346) at org.apache.derby.impl.jdbc.EmbedConnection.handleException(EmbedConnection.java:2269) at org.apache.derby.impl.jdbc.ConnectionChild.handleException(ConnectionChild.java:81) at org.apache.derby.impl.jdbc.EmbedStatement.executeStatement(EmbedStatement.java:1321) at org.apache.derby.impl.jdbc.EmbedPreparedStatement.executeStatement(EmbedPreparedStatement.java:1673) at org.apache.derby.impl.jdbc.EmbedPreparedStatement.executeUpdate(EmbedPreparedStatement.java:303) at org.apache.derbyTesting.functionTests.tests.jdbcapi.StreamErrRepro.test(StreamErrRepro.java:38) at org.apache.derbyTesting.functionTests.tests.jdbcapi.StreamErrRepro.main(StreamErrRepro.java:22) Caused by: java.sql.SQLException: An IOException was thrown when reading a 'java.lang.String' from an InputStream. at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(SQLExceptionFactory.java:45) at org.apache.derby.impl.jdbc.SQLExceptionFactory40.wrapArgsForTransportAcrossDRDA(SQLExceptionFactory40.java:119) at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:70) ... 11 more Caused by: java.sql.SQLException: Java exception: 'Input stream did not have exact amount of data as the requested length.: org.apache.derby.iapi.services.io.DerbyIOException'. at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(SQLExceptionFactory.java:45) at org.apache.derby.impl.jdbc.SQLExceptionFactory40.wrapArgsForTransportAcrossDRDA(SQLExceptionFactory40.java:119) at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:70) at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Util.java:142) at org.apache.derby.impl.jdbc.Util.javaException(Util.java:299) at org.apache.derby.impl.jdbc.TransactionResourceImpl.wrapInSQLException(TransactionResourceImpl.java:403) ... 9 more Caused by: org.apache.derby.iapi.services.io.DerbyIOException: Input stream did not have exact amount of data as the requested length. at org.apache.derby.iapi.types.ReaderToUTF8Stream.checkSufficientData(ReaderToUTF8Stream.java:420) at org.apache.derby.iapi.types.ReaderToUTF8Stream.fillBuffer(ReaderToUTF8Stream.java:378) at org.apache.derby.iapi.types.ReaderToUTF8Stream.read(ReaderToUTF8Stream.java:197) at java.io.DataInputStream.readUnsignedShort(Unknown Source) at org.apache.derby.iapi.types.SQLChar.readExternal(SQLChar.java:1050) at org.apache.derby.iapi.types.SQLChar.getString(SQLChar.java:695) at org.apache.derby.iapi.types.SQLVarchar.normalize(SQLVarchar.java:148) at org.apache.derby.iapi.types.DataTypeDescriptor.normalize(DataTypeDescriptor.java:648) at org.apache.derby.impl.sql.execute.NormalizeResultSet.normalizeColumn(NormalizeResultSet.java:329) at org.apache.derby.impl.sql.execute.NormalizeResultSet.normalizeRow(NormalizeResultSet.java:373) at org.apache.derby.impl.sql.execute.NormalizeResultSet.getNextRowCore(NormalizeResultSet.java:188) at org.apache.derby.impl.sql.execute.DMLWriteResultSet.getNextRowCore(DMLWriteResultSet.java:127) at org.apache.derby.impl.sql.execute.InsertResultSet.open(InsertResultSet.java:494) at org.apache.derby.impl.sql.GenericPreparedStatement.executeStmt(GenericPreparedStatement.java:436) at org.apache.derby.impl.sql.GenericPreparedStatement.execute(GenericPreparedStatement.java:317) at org.apache.derby.impl.jdbc.EmbedStatement.executeStatement(EmbedStatement.java:1232) ... 4 more ----- SQLException ----- SQLState: XJ001 Error Code: 0 Message: Java exception: 'Input stream did not have exact amount of data as the requested length.: org.apache.derby.iapi.services.io.DerbyIOException'. java.sql.SQLException: Java exception: 'Input stream did not have exact amount of data as the requested length.: org.apache.derby.iapi.services.io.DerbyIOException'. at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:95) at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Util.java:142) at org.apache.derby.impl.jdbc.Util.javaException(Util.java:299) at org.apache.derby.impl.jdbc.TransactionResourceImpl.wrapInSQLException(TransactionResourceImpl.java:403) at org.apache.derby.impl.jdbc.TransactionResourceImpl.wrapInSQLException(TransactionResourceImpl.java:398) at org.apache.derby.impl.jdbc.TransactionResourceImpl.handleException(TransactionResourceImpl.java:346) at org.apache.derby.impl.jdbc.EmbedConnection.handleException(EmbedConnection.java:2269) at org.apache.derby.impl.jdbc.ConnectionChild.handleException(ConnectionChild.java:81) at org.apache.derby.impl.jdbc.EmbedStatement.executeStatement(EmbedStatement.java:1321) at org.apache.derby.impl.jdbc.EmbedPreparedStatement.executeStatement(EmbedPreparedStatement.java:1673) at org.apache.derby.impl.jdbc.EmbedPreparedStatement.executeUpdate(EmbedPreparedStatement.java:303) at org.apache.derbyTesting.functionTests.tests.jdbcapi.StreamErrRepro.test(StreamErrRepro.java:38) at org.apache.derbyTesting.functionTests.tests.jdbcapi.StreamErrRepro.main(StreamErrRepro.java:22) Caused by: java.sql.SQLException: Java exception: 'Input stream did not have exact amount of data as the requested length.: org.apache.derby.iapi.services.io.DerbyIOException'. at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(SQLExceptionFactory.java:45) at org.apache.derby.impl.jdbc.SQLExceptionFactory40.wrapArgsForTransportAcrossDRDA(SQLExceptionFactory40.java:119) at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:70) ... 12 more Caused by: org.apache.derby.iapi.services.io.DerbyIOException: Input stream did not have exact amount of data as the requested length. at org.apache.derby.iapi.types.ReaderToUTF8Stream.checkSufficientData(ReaderToUTF8Stream.java:420) at org.apache.derby.iapi.types.ReaderToUTF8Stream.fillBuffer(ReaderToUTF8Stream.java:378) at org.apache.derby.iapi.types.ReaderToUTF8Stream.read(ReaderToUTF8Stream.java:197) at java.io.DataInputStream.readUnsignedShort(Unknown Source) at org.apache.derby.iapi.types.SQLChar.readExternal(SQLChar.java:1050) at org.apache.derby.iapi.types.SQLChar.getString(SQLChar.java:695) at org.apache.derby.iapi.types.SQLVarchar.normalize(SQLVarchar.java:148) at org.apache.derby.iapi.types.DataTypeDescriptor.normalize(DataTypeDescriptor.java:648) at org.apache.derby.impl.sql.execute.NormalizeResultSet.normalizeColumn(NormalizeResultSet.java:329) at org.apache.derby.impl.sql.execute.NormalizeResultSet.normalizeRow(NormalizeResultSet.java:373) at org.apache.derby.impl.sql.execute.NormalizeResultSet.getNextRowCore(NormalizeResultSet.java:188) at org.apache.derby.impl.sql.execute.DMLWriteResultSet.getNextRowCore(DMLWriteResultSet.java:127) at org.apache.derby.impl.sql.execute.InsertResultSet.open(InsertResultSet.java:494) at org.apache.derby.impl.sql.GenericPreparedStatement.executeStmt(GenericPreparedStatement.java:436) at org.apache.derby.impl.sql.GenericPreparedStatement.execute(GenericPreparedStatement.java:317) at org.apache.derby.impl.jdbc.EmbedStatement.executeStatement(EmbedStatement.java:1232) ... 4 more When using the client driver, the following stack trace is written to derby.log: ============= begin nested exception, level (1) =========== org.apache.derby.iapi.services.io.DerbyIOException: Input stream did not have exact amount of data as the requested length. at org.apache.derby.impl.drda.EXTDTAReaderInputStream.checkStatus(EXTDTAReaderInputStream.java:173) at org.apache.derby.impl.drda.StandardEXTDTAReaderInputStream.read(StandardEXTDTAReaderInputStream.java:146) at sun.nio.cs.StreamDecoder.readBytes(Unknown Source) at sun.nio.cs.StreamDecoder.implRead(Unknown Source) at sun.nio.cs.StreamDecoder.read(Unknown Source) at sun.nio.cs.StreamDecoder.read0(Unknown Source) at sun.nio.cs.StreamDecoder.read(Unknown Source) at java.io.InputStreamReader.read(Unknown Source) at org.apache.derby.iapi.services.io.LimitReader.read(LimitReader.java:57) at org.apache.derby.iapi.types.ReaderToUTF8Stream.fillBuffer(ReaderToUTF8Stream.java:352) at org.apache.derby.iapi.types.ReaderToUTF8Stream.read(ReaderToUTF8Stream.java:197) at java.io.DataInputStream.readUnsignedShort(Unknown Source) at org.apache.derby.iapi.types.SQLChar.readExternal(SQLChar.java:1050) at org.apache.derby.iapi.types.SQLChar.getString(SQLChar.java:695) at org.apache.derby.iapi.types.SQLVarchar.normalize(SQLVarchar.java:148) at org.apache.derby.iapi.types.DataTypeDescriptor.normalize(DataTypeDescriptor.java:648) at org.apache.derby.impl.sql.execute.NormalizeResultSet.normalizeColumn(NormalizeResultSet.java:329) at org.apache.derby.impl.sql.execute.NormalizeResultSet.normalizeRow(NormalizeResultSet.java:373) at org.apache.derby.impl.sql.execute.NormalizeResultSet.getNextRowCore(NormalizeResultSet.java:188) at org.apache.derby.impl.sql.execute.DMLWriteResultSet.getNextRowCore(DMLWriteResultSet.java:127) at org.apache.derby.impl.sql.execute.InsertResultSet.open(InsertResultSet.java:494) at org.apache.derby.impl.sql.GenericPreparedStatement.executeStmt(GenericPreparedStatement.java:436) at org.apache.derby.impl.sql.GenericPreparedStatement.execute(GenericPreparedStatement.java:317) at org.apache.derby.impl.jdbc.EmbedStatement.executeStatement(EmbedStatement.java:1232) at org.apache.derby.impl.jdbc.EmbedPreparedStatement.executeStatement(EmbedPreparedStatement.java:1673) at org.apache.derby.impl.jdbc.EmbedPreparedStatement.execute(EmbedPreparedStatement.java:1328) at org.apache.derby.impl.drda.DRDAStatement.execute(DRDAStatement.java:672) at org.apache.derby.impl.drda.DRDAConnThread.parseEXCSQLSTTobjects(DRDAConnThread.java:4262) at org.apache.derby.impl.drda.DRDAConnThread.parseEXCSQLSTT(DRDAConnThread.java:4085) at org.apache.derby.impl.drda.DRDAConnThread.processCommands(DRDAConnThread.java:1004) at org.apache.derby.impl.drda.DRDAConnThread.run(DRDAConnThread.java:291) ============= end nested exception, level (1) =========== If we disable the use of the accumulated exception mechanism on the client (when sending the status byte, i.e. server and client at 10.6+), we get the same SQL states (XCL30 and XJ001) in both drivers. Looking at the traces, I do see DerbyIOException. This was introduced to make Derby able to rewrite IOException to SQLException with the correct state, but this is obviously not done for this code path. For instance, we may want to differentiate between a generic read error from a user stream and an error thrown by Derby due to a mismatch between the specified and the actual stream length. I would prefer to throw a different exception than the generic XJ001, but I think this can be handled under a different Jira. For instance, XJ023 may be better suited.
          Hide
          Knut Anders Hatlen added a comment -

          I agree that it would be good to get the same error on both drivers for the situations above.

          The situation I had in mind was slightly different. I was thinking of the case where the user stream throws an exception when it's read. In the current code, I believe that both the client driver and the embedded driver will expose the original exception thrown by the user stream. If we disable the accumulation of exceptions, will we then instead see the below exception on the client?

          + case DRDAConstants.STREAM_READ_ERROR:
          + case DRDAConstants.STREAM_READ_ERROR_ON_LEN_VAL:
          + throw new IOException("Read error on client side when " +
          + "reading user stream");

          My preference would be that we continued to report the original exception in such a situation.

          Show
          Knut Anders Hatlen added a comment - I agree that it would be good to get the same error on both drivers for the situations above. The situation I had in mind was slightly different. I was thinking of the case where the user stream throws an exception when it's read. In the current code, I believe that both the client driver and the embedded driver will expose the original exception thrown by the user stream. If we disable the accumulation of exceptions, will we then instead see the below exception on the client? + case DRDAConstants.STREAM_READ_ERROR: + case DRDAConstants.STREAM_READ_ERROR_ON_LEN_VAL: + throw new IOException("Read error on client side when " + + "reading user stream"); My preference would be that we continued to report the original exception in such a situation.
          Hide
          Kristian Waagan added a comment -

          Attached an updated version of the regression test patch, which adds another test class (for layer B).

          I plan to commit this shortly, and I will do a few changes once the matter of exception handling in the drivers has been settled.
          Run on trunk 5+3 failures should be seen. There are a few cases where the client driver currently does ok (because a combination of no auto-commit and rollback is used), and most of the tests are also run with the embedded driver as verification.

          Show
          Kristian Waagan added a comment - Attached an updated version of the regression test patch, which adds another test class (for layer B). I plan to commit this shortly, and I will do a few changes once the matter of exception handling in the drivers has been settled. Run on trunk 5+3 failures should be seen. There are a few cases where the client driver currently does ok (because a combination of no auto-commit and rollback is used), and most of the tests are also run with the embedded driver as verification.
          Hide
          Kristian Waagan added a comment -

          Committed patch 2b to trunk with revision 927114.

          Show
          Kristian Waagan added a comment - Committed patch 2b to trunk with revision 927114.
          Hide
          Kristian Waagan added a comment -

          Attached a fix as patch 3a.

          Ready for review.
          I'll describe/explain a few things tomorrow (no time now, just wanted to get the patch out there).

          Show
          Kristian Waagan added a comment - Attached a fix as patch 3a. Ready for review. I'll describe/explain a few things tomorrow (no time now, just wanted to get the patch out there).
          Hide
          Kristian Waagan added a comment -

          Description of patch 3a:

          • iapi/reference/DRDAConstants
            Constants for the various error conditions. There are currently four different conditions: OK, READ_ERROR, TOO_SHORT and TOO_LONG.
            Two would be enough, but having more may help debugging / tracing (from the server side) and it doesn't add much complexity / overhead.
          • drda/StandardEXTDTAReaderInputStream
            Extended the stream reading "normal" EXTDTA to handle the Derby-specific status byte.
            See comment under drda/EXTDTAReaderInputStream.
          • drda/LayerBStreamedEXTDTAReaderInputStream
            Extended the stream reading layer B streamed EXTDTA to handle the Derby-specific status byte.
            Slightly more complex than the normal case, as we don't know up front when we read the last data byte.
            First I tried to make the stream check the status when it read the last byte, but the code got too complex due to buffer boundary issues (i.e. I had to deal with a "carry-over" byte etc). The stream now requires that it is properly drained to guarantee that the status byte is read (i.e. read in a loop until you get -1). This is the normal operational mode, so it shouldn't cause problems.
            To my knowledge this stream is always passed directly into the embedded driver, which means it should thrown an exception instead of saving the status byte.
          • drda/EXTDTAReaderInputStream
            Extended the class to deal with the Derby-specific status byte.
            Contains the logic determining if something went wrong when reading the stream on the client side. If so, either an exception is thrown or the status if saved for later inspection.
            Seems I removed some required functionality here, as I think we have to be able to explicitly tell if an error condition should cause an exception to be thrown or not. The reason is that sometimes Derby reads the value off the network before executing the statement. I want the statement itself to fail, as this simplifies the clean-up and error handling.
          • drda/FailingEXTDTAInputStream
            Dummy stream that will throw an exception as soon as a read request is made.
          • drda/AppRequester
            Added method that tells if the client supports EXTDTA aborts or not.
          • drda/DRDAConnThread
            Fixed some suboptimal logic in convertAsByteArrayInputStream, where the buffer size could be set to poorly chosen values (very small or too large).
            In the case of normal EXTDTAs, added logic to pass in a stream that will fail instead of the stream containing padded or truncated data.
          • drda/DDMReader
            Modified the code creating the streams reading data off the wire to tell whether a Derby-specific status byte is expected or not.
          • tests/jdbc4/PreparedStatementTest
            Removed special code that was added due to DERBY-2017.
          • client/net/NetDatabaseMetaData
            Added code to tell if the server supports EXTDTA aborts.
          • client/net/NetConnection
            Added method calling code added in NetDatabaseMetaData.
          • client/net/Request
            The changes required to send the status byte (both for layer A and layer B streaming).
            Note that I suspect that the method writeEncryptedScalarStream is dead code. There are several major problems with it, suggesting that it isn't used. I may remove this code in a later patch, added a comment for now.

          I'll add some more JavaDoc for some of the existing methods in Request, add some tests for binary data (as opposed to character data) and verify that all code paths in DRDAConnThread.readAndSetExtParam are covered.

          Show
          Kristian Waagan added a comment - Description of patch 3a: iapi/reference/DRDAConstants Constants for the various error conditions. There are currently four different conditions: OK, READ_ERROR, TOO_SHORT and TOO_LONG. Two would be enough, but having more may help debugging / tracing (from the server side) and it doesn't add much complexity / overhead. drda/StandardEXTDTAReaderInputStream Extended the stream reading "normal" EXTDTA to handle the Derby-specific status byte. See comment under drda/EXTDTAReaderInputStream. drda/LayerBStreamedEXTDTAReaderInputStream Extended the stream reading layer B streamed EXTDTA to handle the Derby-specific status byte. Slightly more complex than the normal case, as we don't know up front when we read the last data byte. First I tried to make the stream check the status when it read the last byte, but the code got too complex due to buffer boundary issues (i.e. I had to deal with a "carry-over" byte etc). The stream now requires that it is properly drained to guarantee that the status byte is read (i.e. read in a loop until you get -1). This is the normal operational mode, so it shouldn't cause problems. To my knowledge this stream is always passed directly into the embedded driver, which means it should thrown an exception instead of saving the status byte. drda/EXTDTAReaderInputStream Extended the class to deal with the Derby-specific status byte. Contains the logic determining if something went wrong when reading the stream on the client side. If so, either an exception is thrown or the status if saved for later inspection. Seems I removed some required functionality here, as I think we have to be able to explicitly tell if an error condition should cause an exception to be thrown or not. The reason is that sometimes Derby reads the value off the network before executing the statement. I want the statement itself to fail, as this simplifies the clean-up and error handling. drda/FailingEXTDTAInputStream Dummy stream that will throw an exception as soon as a read request is made. drda/AppRequester Added method that tells if the client supports EXTDTA aborts or not. drda/DRDAConnThread Fixed some suboptimal logic in convertAsByteArrayInputStream, where the buffer size could be set to poorly chosen values (very small or too large). In the case of normal EXTDTAs, added logic to pass in a stream that will fail instead of the stream containing padded or truncated data. drda/DDMReader Modified the code creating the streams reading data off the wire to tell whether a Derby-specific status byte is expected or not. tests/jdbc4/PreparedStatementTest Removed special code that was added due to DERBY-2017 . client/net/NetDatabaseMetaData Added code to tell if the server supports EXTDTA aborts. client/net/NetConnection Added method calling code added in NetDatabaseMetaData. client/net/Request The changes required to send the status byte (both for layer A and layer B streaming). Note that I suspect that the method writeEncryptedScalarStream is dead code. There are several major problems with it, suggesting that it isn't used. I may remove this code in a later patch, added a comment for now. I'll add some more JavaDoc for some of the existing methods in Request, add some tests for binary data (as opposed to character data) and verify that all code paths in DRDAConnThread.readAndSetExtParam are covered.
          Hide
          Knut Anders Hatlen added a comment -

          Hi Kristian,

          I downloaded the 3a patch and tested it, and it only seems to address the layer B case (as can be seen by running Derby2017LayerATest or the original repro which still fail). Was layer A supposed to work with this patch?

          Show
          Knut Anders Hatlen added a comment - Hi Kristian, I downloaded the 3a patch and tested it, and it only seems to address the layer B case (as can be seen by running Derby2017LayerATest or the original repro which still fail). Was layer A supposed to work with this patch?
          Hide
          Knut Anders Hatlen added a comment -

          The subclasses of EXTDTAReaderInputStream implement both read() and read(byte[],int,int). Those methods have become rather complex now, since they handle both switching of buffers and the new status byte. To reduce the complexity of these classes, do you think it would make sense to define read() in terms of read(byte[],int,int) so that buffer switching and reading the status byte only need to be implemented once per class?

          Show
          Knut Anders Hatlen added a comment - The subclasses of EXTDTAReaderInputStream implement both read() and read(byte[],int,int). Those methods have become rather complex now, since they handle both switching of buffers and the new status byte. To reduce the complexity of these classes, do you think it would make sense to define read() in terms of read(byte[],int,int) so that buffer switching and reading the status byte only need to be implemented once per class?
          Hide
          Kristian Waagan added a comment -

          Hi Knut Anders,

          Here's another version. 3a had a bug in the checkState-method (I removed some functionality I thought was unnecessary), which is why Derby2017LayerATest failed.
          I have also made some additional changes in DRDAConnThread (refactoring). See DRDAConnThread.convertAsByteArrayInputStream for the use of the exception suppress mechanism in the streaming classes.
          Added some more JavaDoc to the methods in Request.

          I also made the change you suggested - to implement read() using read(byte[], int, int). It does reduce the complexity, and I know that the normal access pattern for these stream classes is to read data into a buffer (we're currently using a 8 KB buffer). I considered keeping the one-byte array as an instance variable, but chose not to.

          I just started the regression tests, and will report back later with the results.
          More tests are coming (using setBinaryStream).

          Patch 3b ready for further review.

          Show
          Kristian Waagan added a comment - Hi Knut Anders, Here's another version. 3a had a bug in the checkState-method (I removed some functionality I thought was unnecessary), which is why Derby2017LayerATest failed. I have also made some additional changes in DRDAConnThread (refactoring). See DRDAConnThread.convertAsByteArrayInputStream for the use of the exception suppress mechanism in the streaming classes. Added some more JavaDoc to the methods in Request. I also made the change you suggested - to implement read() using read(byte[], int, int). It does reduce the complexity, and I know that the normal access pattern for these stream classes is to read data into a buffer (we're currently using a 8 KB buffer). I considered keeping the one-byte array as an instance variable, but chose not to. I just started the regression tests, and will report back later with the results. More tests are coming (using setBinaryStream). Patch 3b ready for further review.
          Hide
          Knut Anders Hatlen added a comment -

          Thanks, Kristian. The new patch looks fine to me. My only further comments are (I don't think you need to hold the commit, though):

          1) New error messages in EXTDTAReaderInputStream.getStatus()/checkStatus() and FailingEXTDTAInputStream.read() are not internationalized.

          2) Could the two JUnit tests for this issue also be enabled now?

          By the way, I noticed that the layer B test ran in just 6 seconds on my laptop, whereas the layer A test needed 2 minutes to complete. I haven't studied the details of the tests, but perhaps you could tell if this difference is expected?

          Show
          Knut Anders Hatlen added a comment - Thanks, Kristian. The new patch looks fine to me. My only further comments are (I don't think you need to hold the commit, though): 1) New error messages in EXTDTAReaderInputStream.getStatus()/checkStatus() and FailingEXTDTAInputStream.read() are not internationalized. 2) Could the two JUnit tests for this issue also be enabled now? By the way, I noticed that the layer B test ran in just 6 seconds on my laptop, whereas the layer A test needed 2 minutes to complete. I haven't studied the details of the tests, but perhaps you could tell if this difference is expected?
          Hide
          Kristian Waagan added a comment -

          Attached patch 3a.
          Committed the fix to trunk with revision 936950.

          Thanks for looking at the patch, Knut.
          I rewrote it slightly, adding a static method instead of duplicating the logic to throw the exception. I also one of the messages to MessageId. The others shouldn't be seen by users...

          I will post a follow-up patch for the tests. It will add some tests for binary data, and also enable the tests.
          The time difference you see between the two tests is expected. The layer A test runs a buffer boundary test, which is not run in the layer B test (the two tests exercise the same code path when it comes to the buffer boundary issue).

          If there are additional comments, I'll incorporate the necessary changes in a new patch.

          Show
          Kristian Waagan added a comment - Attached patch 3a. Committed the fix to trunk with revision 936950. Thanks for looking at the patch, Knut. I rewrote it slightly, adding a static method instead of duplicating the logic to throw the exception. I also one of the messages to MessageId. The others shouldn't be seen by users... I will post a follow-up patch for the tests. It will add some tests for binary data, and also enable the tests. The time difference you see between the two tests is expected. The layer A test runs a buffer boundary test, which is not run in the layer B test (the two tests exercise the same code path when it comes to the buffer boundary issue). If there are additional comments, I'll incorporate the necessary changes in a new patch.
          Hide
          Knut Anders Hatlen added a comment -

          There were two failures in the Tinderbox after this check-in. Do you think they are related?

          http://dbtg.foundry.sun.com/derby/test/tinderbox_trunk16/jvm1.6/testing/testlog/SunOS-5.10_i86pc-i386/936985-org.apache.derbyTesting.functionTests.suites.All_diff.txt

          org.apache.derbyTesting.functionTests.suites.All fail *************************************************************
          1) testBlobExceptionDoesNotRollbackOtherStatements(org.apache.derbyTesting.functionTests.tests.jdbc4.PreparedStatementTest)junit.framework.AssertionFailedError
          2) testBlobExceptionDoesNotRollbackOtherStatements(org.apache.derbyTesting.functionTests.tests.jdbc4.PreparedStatementTest)junit.framework.AssertionFailedError

          Show
          Knut Anders Hatlen added a comment - There were two failures in the Tinderbox after this check-in. Do you think they are related? http://dbtg.foundry.sun.com/derby/test/tinderbox_trunk16/jvm1.6/testing/testlog/SunOS-5.10_i86pc-i386/936985-org.apache.derbyTesting.functionTests.suites.All_diff.txt org.apache.derbyTesting.functionTests.suites.All fail ************************************************************* 1) testBlobExceptionDoesNotRollbackOtherStatements(org.apache.derbyTesting.functionTests.tests.jdbc4.PreparedStatementTest)junit.framework.AssertionFailedError 2) testBlobExceptionDoesNotRollbackOtherStatements(org.apache.derbyTesting.functionTests.tests.jdbc4.PreparedStatementTest)junit.framework.AssertionFailedError
          Hide
          Kristian Waagan added a comment -

          Absolutely.

          Going from 3b to 3c the test fix got lost.
          Attached as patch 4a, committed to trunk with revision 937198.

          Show
          Kristian Waagan added a comment - Absolutely. Going from 3b to 3c the test fix got lost. Attached as patch 4a, committed to trunk with revision 937198.
          Hide
          Kristian Waagan added a comment -

          Attached patch 5a, which adds tests of binary data values (slightly different code path on the server) and enables the tests as part of their respective suites.

          Patch ready for review.
          Will commit shortly.

          Show
          Kristian Waagan added a comment - Attached patch 5a, which adds tests of binary data values (slightly different code path on the server) and enables the tests as part of their respective suites. Patch ready for review. Will commit shortly.
          Hide
          Kristian Waagan added a comment -

          Committed patch 5a to trunk with revision 937655.

          Show
          Kristian Waagan added a comment - Committed patch 5a to trunk with revision 937655.
          Hide
          Kristian Waagan added a comment -

          Although the error reporting could have been cleaned up somehow (change error message text, make states consistent between the drivers), I don't expect to do more work on this issue for 10.6.

          Fix ready for verification.

          Show
          Kristian Waagan added a comment - Although the error reporting could have been cleaned up somehow (change error message text, make states consistent between the drivers), I don't expect to do more work on this issue for 10.6. Fix ready for verification.
          Hide
          Kathey Marsden added a comment -

          I looked at this issue for possible backport to 10.5 and think it is not appropriate because there were pretty extensive changes and protocol changes. From the commit message of r936950:

          "Made the client tell the server (if supported) if the EXTDTA transfer was
          successful or not, by appending a Derby-specific status byte to the user data."

          Show
          Kathey Marsden added a comment - I looked at this issue for possible backport to 10.5 and think it is not appropriate because there were pretty extensive changes and protocol changes. From the commit message of r936950: "Made the client tell the server (if supported) if the EXTDTA transfer was successful or not, by appending a Derby-specific status byte to the user data."

            People

            • Assignee:
              Kristian Waagan
              Reporter:
              Knut Anders Hatlen
            • Votes:
              1 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development