Derby
  1. Derby
  2. DERBY-5847

Clean up IDE warnings in DRDAConnThread

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.10.1.1
    • Fix Version/s: 10.10.1.1
    • Component/s: Network Server
    • Labels:
      None

      Description

      When I open DRDAConnThread in NetBeans, I see 49 warnings. Most of them are harmless (like static fields accessed via an instance, suggestions about using StringBuilder instead of StringBuffer, or using System.arraycopy() instead of for loops). Others indicate real problems, like the use of != to compare SQL states in writeSQLDIAGGRP().

      We should clean up the warnings so that it's easier to notice new warnings about potential problems.

      1. d5847-9b-this-leak.patch
        5 kB
        Knut Anders Hatlen
      2. d5847-9a-this-leak.patch
        5 kB
        Knut Anders Hatlen
      3. d5847-8b-misc.patch
        2 kB
        Knut Anders Hatlen
      4. d5847-8a-misc.patch
        3 kB
        Knut Anders Hatlen
      5. d5847-7a-sync-on-non-final.patch
        3 kB
        Knut Anders Hatlen
      6. d5847-6a-obsolete-collection.patch
        2 kB
        Knut Anders Hatlen
      7. d5847-5a-performance-warnings.patch
        5 kB
        Knut Anders Hatlen
      8. d5847-4a-unused-assignment.patch
        8 kB
        Knut Anders Hatlen
      9. d5847-3a-static-fields-and-imports.patch
        11 kB
        Knut Anders Hatlen
      10. d5847-2a-unnecessary-return.patch
        2 kB
        Knut Anders Hatlen
      11. d5847-1a-string-equality.patch
        2 kB
        Knut Anders Hatlen
      12. d5847-10a-overrides-and-braces.patch
        107 kB
        Knut Anders Hatlen

        Issue Links

          Activity

          Gavin made changes -
          Workflow jira [ 12710204 ] Default workflow, editable Closed status [ 12797121 ]
          Knut Anders Hatlen made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Knut Anders Hatlen made changes -
          Status In Progress [ 3 ] Resolved [ 5 ]
          Fix Version/s 10.10.0.0 [ 12321550 ]
          Resolution Fixed [ 1 ]
          Hide
          Knut Anders Hatlen added a comment -

          Committed revision 1363661.

          I don't see any more warnings in this file, so marking the issue as resolved.

          Show
          Knut Anders Hatlen added a comment - Committed revision 1363661. I don't see any more warnings in this file, so marking the issue as resolved.
          Knut Anders Hatlen made changes -
          Attachment d5847-10a-overrides-and-braces.patch [ 12537241 ]
          Hide
          Knut Anders Hatlen added a comment -

          Here's a patch (d5847-10a-overrides-and-braces.patch) that cleans up the last batch of warnings that I see in my environment.

          The patch adds an @Override annotation to the run() method, and addresses the warnings about missing braces by adding braces or using existing helper methods (like getDbName(), System.arraycopy() and Math.min()/max()) that make it possible to eliminate the if/else statements and for loops that lack braces. It also collapses an if/else statement in writeFdocaVal() with special cases for Byte, Short an Integer into a single statement that uses the common super-class Number.

          All the regression tests passed with the patch.

          Show
          Knut Anders Hatlen added a comment - Here's a patch (d5847-10a-overrides-and-braces.patch) that cleans up the last batch of warnings that I see in my environment. The patch adds an @Override annotation to the run() method, and addresses the warnings about missing braces by adding braces or using existing helper methods (like getDbName(), System.arraycopy() and Math.min()/max()) that make it possible to eliminate the if/else statements and for loops that lack braces. It also collapses an if/else statement in writeFdocaVal() with special cases for Byte, Short an Integer into a single statement that uses the common super-class Number. All the regression tests passed with the patch.
          Hide
          Knut Anders Hatlen added a comment -

          Committed d5847-6a-obsolete-collection.patch to trunk with revision 1360133.
          Committed d5847-7a-sync-on-non-final.patch to trunk with revision 1360134.
          Committed d5847-8b-misc.patch to trunk with revision 1360135.
          Committed d5847-9b-this-leak.patch to trunk with revision 1360137.

          Now the only warnings left to clean up in this file are:

          • 345 if/else statements without braces
          • 10 for loops without braces
          • 1 missing @Override annotation

          The latter cannot be fixed without starting to compile the DRDA code with source level 1.5. That's probably a good idea anyway, since the DRDA code on trunk isn't supported on earlier Java versions.

          Show
          Knut Anders Hatlen added a comment - Committed d5847-6a-obsolete-collection.patch to trunk with revision 1360133. Committed d5847-7a-sync-on-non-final.patch to trunk with revision 1360134. Committed d5847-8b-misc.patch to trunk with revision 1360135. Committed d5847-9b-this-leak.patch to trunk with revision 1360137. Now the only warnings left to clean up in this file are: 345 if/else statements without braces 10 for loops without braces 1 missing @Override annotation The latter cannot be fixed without starting to compile the DRDA code with source level 1.5. That's probably a good idea anyway, since the DRDA code on trunk isn't supported on earlier Java versions.
          Knut Anders Hatlen made changes -
          Attachment d5847-8b-misc.patch [ 12536019 ]
          Hide
          Knut Anders Hatlen added a comment -

          Replacing the 8a-misc patch with d5847-8b-misc.patch. The new version removes the dead code and adds the following comment:

          // The protocol wants us to send RDBUPDRM here, but we don't do
          // that because it used to cause protocol errors. DERBY-5847 has
          // some discussion about this issue.

          It's a bit vague, since we don't know exactly which errors the code caused.

          For the record, I tried to enable the code and ran some statements that exercised the path. That is, the following commands in ij:

          ij> connect 'jdbc:derby://localhost/db;create=true';
          ij> create table t (x int);
          0 rows inserted/updated/deleted
          ij> prepare ps as 'insert into t values 1,2,3';
          ij> execute ps;
          3 rows inserted/updated/deleted
          ij> execute ps;
          3 rows inserted/updated/deleted

          I verified that RDBUPDRM was sent on the "execute ps" statements, and no protocol error was seen.

          I also repeated the experiment using the JCC driver (had to make the changes in a 10.8 server to get it to run, as trunk rejects JCC after DERBY-5565). Same result there; RDBUPDRM was sent, and the JCC client happily accepted it.

          So it may be OK to start sending RDBUPDRM now. But there's also a chance that there's some other usage pattern that will trigger the mentioned protocol error. Since there are no known problems caused by the lack of the RDBUPDRM, and this issue is about silencing warnings, I'll just leave it like that for now.

          Show
          Knut Anders Hatlen added a comment - Replacing the 8a-misc patch with d5847-8b-misc.patch. The new version removes the dead code and adds the following comment: // The protocol wants us to send RDBUPDRM here, but we don't do // that because it used to cause protocol errors. DERBY-5847 has // some discussion about this issue. It's a bit vague, since we don't know exactly which errors the code caused. For the record, I tried to enable the code and ran some statements that exercised the path. That is, the following commands in ij: ij> connect 'jdbc:derby://localhost/db;create=true'; ij> create table t (x int); 0 rows inserted/updated/deleted ij> prepare ps as 'insert into t values 1,2,3'; ij> execute ps; 3 rows inserted/updated/deleted ij> execute ps; 3 rows inserted/updated/deleted I verified that RDBUPDRM was sent on the "execute ps" statements, and no protocol error was seen. I also repeated the experiment using the JCC driver (had to make the changes in a 10.8 server to get it to run, as trunk rejects JCC after DERBY-5565 ). Same result there; RDBUPDRM was sent, and the JCC client happily accepted it. So it may be OK to start sending RDBUPDRM now. But there's also a chance that there's some other usage pattern that will trigger the mentioned protocol error. Since there are no known problems caused by the lack of the RDBUPDRM, and this issue is about silencing warnings, I'll just leave it like that for now.
          Hide
          Bryan Pendleton added a comment -

          Ah, I see. Either way seems good, then. I guess I have a slight preference toward removing the
          code and replacing it with a command describing the issue, but I'm fine with commenting out
          the code too. Thanks for the explanation.

          Show
          Bryan Pendleton added a comment - Ah, I see. Either way seems good, then. I guess I have a slight preference toward removing the code and replacing it with a command describing the issue, but I'm fine with commenting out the code too. Thanks for the explanation.
          Hide
          Knut Anders Hatlen added a comment -

          Thanks, Bryan. In general, I agree that it's better to delete unused code. But in this particular case, I felt that the code had some value documenting the fact that the protocol requires us to send a specific reply message, but because of a bug we cannot do that. (Assuming that the protocol actually requires a RDBUPDRM here, but I haven't checked that.) On the other hand, it might be OK to remove the code and just insert a comment saying that we're violating the protocol.

          Show
          Knut Anders Hatlen added a comment - Thanks, Bryan. In general, I agree that it's better to delete unused code. But in this particular case, I felt that the code had some value documenting the fact that the protocol requires us to send a specific reply message, but because of a bug we cannot do that. (Assuming that the protocol actually requires a RDBUPDRM here, but I haven't checked that.) On the other hand, it might be OK to remove the code and just insert a comment saying that we're violating the protocol.
          Hide
          Bryan Pendleton added a comment -

          Regarding "if" statements which are never executed, my preference is to delete the if statement
          entirely, rather than commenting it out with more comments. We can always recover the old
          code from Subversion should we need it.

          Show
          Bryan Pendleton added a comment - Regarding "if" statements which are never executed, my preference is to delete the if statement entirely, rather than commenting it out with more comments. We can always recover the old code from Subversion should we need it.
          Knut Anders Hatlen made changes -
          Attachment d5847-9b-this-leak.patch [ 12535837 ]
          Hide
          Knut Anders Hatlen added a comment -

          I just noticed a small, but unintentional, change caused by the 9a patch. Previously, threads would have underscores in their names (like DRDAConnThread_3), whereas they now have hyphens (DRDAConnThread-3). Patch 9b changes the names back to the old format. Rerunning tests...

          Show
          Knut Anders Hatlen added a comment - I just noticed a small, but unintentional, change caused by the 9a patch. Previously, threads would have underscores in their names (like DRDAConnThread_3), whereas they now have hyphens (DRDAConnThread-3). Patch 9b changes the names back to the old format. Rerunning tests...
          Knut Anders Hatlen made changes -
          Attachment d5847-6a-obsolete-collection.patch [ 12535829 ]
          Attachment d5847-7a-sync-on-non-final.patch [ 12535830 ]
          Attachment d5847-8a-misc.patch [ 12535831 ]
          Attachment d5847-9a-this-leak.patch [ 12535832 ]
          Hide
          Knut Anders Hatlen added a comment -

          Attaching four patches that address various warnings. I'll keep them as separate patches so that it's easier to review the changes, or back out parts of them later if some of the changes cause problems. The regression tests passed.

          Here's what the patches do:

          • d5847-6a-obsolete-collection.patch:

          Replace use of obsolete collection class Vector with ArrayList. No synchronization is needed, as far as I can see, since the vectors are private to a single server thread.

          Also converted two of the original Vector fields (errorManagers and errorManagersLevel) to local variables, as they are never used outside of the method where they are created.

          • d5847-7a-sync-on-non-final.patch:

          NetBeans warns about synchronization on some non-final fields: timeSliceSync, logConnectionsSync and closeSync, which protect the fields timeSlice, logConnections and close, respectively.

          The easiest fix would be to make the *Sync fields final. However, the synchronization objects are only used in set/get methods to ensure that writes in one thread are seen by reads in other threads. For this purpose, using volatile fields would be sufficient, and the extra fields with synchronization objects could be removed. The patch therefore declares timeSlice, logConnections and close volatile, and it removes the timeSliceSync, logConnectionsSync and closeSync fields.

          • d5847-8a-misc.patch:

          This patch fixes three warnings:

          • An if statement had been disabled by prepending "false &&" to the condition, so it never evaluated to true. A warning was shown because the code in the body of the if statement was unreachable. The patch comments out the code, and also adds a comment about why the code was disabled in the first place.
          • An unnecessary bit shift operation was removed (shift bits 0 positions to the left is a no-op).
          • A local variable in setDatabase() hid a field and was renamed from rdbnam to dbname.
          • d5847-9a-this-leak.patch:

          The warning that it addresses is about the constructor leaking a reference to "this". That is, the constructor calls a method and passes "this" as an argument. The problem is that the method to which "this" is passed sees a not yet fully initialized instance. This may or may not be a problem depending on which parts of the state the method needs to access, but it's best avoided.

          The patch changes the name and signature of the method that receives the reference to "this", from NetworkServerControlImpl.setUniqueThreadName(Thread thrd, String newName) to NetworkServerControlImpl.getUniqueThreadName(String base). The new method returns a unique thread name instead of setting the name of thread. The constructor could therefore pass the returned value to the super constructor instead of passing a reference to itself to the setUniqueThreadName() method. In addition to silencing the warning, this approach has the advantages that it's less code and the thread name doesn't have to be set twice per thread.

          A similar change had to be made to ClientThread's constructor, since NetworkServerControlImpl.setUniqueThreadName() was also used there.

          Show
          Knut Anders Hatlen added a comment - Attaching four patches that address various warnings. I'll keep them as separate patches so that it's easier to review the changes, or back out parts of them later if some of the changes cause problems. The regression tests passed. Here's what the patches do: d5847-6a-obsolete-collection.patch: Replace use of obsolete collection class Vector with ArrayList. No synchronization is needed, as far as I can see, since the vectors are private to a single server thread. Also converted two of the original Vector fields (errorManagers and errorManagersLevel) to local variables, as they are never used outside of the method where they are created. d5847-7a-sync-on-non-final.patch: NetBeans warns about synchronization on some non-final fields: timeSliceSync, logConnectionsSync and closeSync, which protect the fields timeSlice, logConnections and close, respectively. The easiest fix would be to make the *Sync fields final. However, the synchronization objects are only used in set/get methods to ensure that writes in one thread are seen by reads in other threads. For this purpose, using volatile fields would be sufficient, and the extra fields with synchronization objects could be removed. The patch therefore declares timeSlice, logConnections and close volatile, and it removes the timeSliceSync, logConnectionsSync and closeSync fields. d5847-8a-misc.patch: This patch fixes three warnings: An if statement had been disabled by prepending "false &&" to the condition, so it never evaluated to true. A warning was shown because the code in the body of the if statement was unreachable. The patch comments out the code, and also adds a comment about why the code was disabled in the first place. An unnecessary bit shift operation was removed (shift bits 0 positions to the left is a no-op). A local variable in setDatabase() hid a field and was renamed from rdbnam to dbname. d5847-9a-this-leak.patch: The warning that it addresses is about the constructor leaking a reference to "this". That is, the constructor calls a method and passes "this" as an argument. The problem is that the method to which "this" is passed sees a not yet fully initialized instance. This may or may not be a problem depending on which parts of the state the method needs to access, but it's best avoided. The patch changes the name and signature of the method that receives the reference to "this", from NetworkServerControlImpl.setUniqueThreadName(Thread thrd, String newName) to NetworkServerControlImpl.getUniqueThreadName(String base). The new method returns a unique thread name instead of setting the name of thread. The constructor could therefore pass the returned value to the super constructor instead of passing a reference to itself to the setUniqueThreadName() method. In addition to silencing the warning, this approach has the advantages that it's less code and the thread name doesn't have to be set twice per thread. A similar change had to be made to ClientThread's constructor, since NetworkServerControlImpl.setUniqueThreadName() was also used there.
          Knut Anders Hatlen made changes -
          Attachment d5847-5a-performance-warnings.patch [ 12535815 ]
          Hide
          Knut Anders Hatlen added a comment -

          The patch d5847-5a-performance-warnings.patch addresses a batch of warnings that NetBeans classifies as performance warnings. None of the warnings appear to be in particularly performance critical code, but they still look like good suggestions, I think. Three different kinds of warnings are addressed by the patch:

          • System.arraycopy() should be used instead of manual loops where possible.
          • StringBuilder should be used instead of StringBuffer if synchronization is not required.
          • String concatenation should not happen in the arguments to StringBuffer.append(). Instead, there should be multiple calls to append().

          The patch changes the code as suggested. In addition, it eliminates the use of StringBuffer in convertToHexString() by using the utility method StringUtil.toHexString().

          Tests passed. Committed revision 1359635.

          Show
          Knut Anders Hatlen added a comment - The patch d5847-5a-performance-warnings.patch addresses a batch of warnings that NetBeans classifies as performance warnings. None of the warnings appear to be in particularly performance critical code, but they still look like good suggestions, I think. Three different kinds of warnings are addressed by the patch: System.arraycopy() should be used instead of manual loops where possible. StringBuilder should be used instead of StringBuffer if synchronization is not required. String concatenation should not happen in the arguments to StringBuffer.append(). Instead, there should be multiple calls to append(). The patch changes the code as suggested. In addition, it eliminates the use of StringBuffer in convertToHexString() by using the utility method StringUtil.toHexString(). Tests passed. Committed revision 1359635.
          Knut Anders Hatlen made changes -
          Attachment d5847-4a-unused-assignment.patch [ 12535812 ]
          Hide
          Knut Anders Hatlen added a comment -

          Attaching d5847-4a-unused-assignment.patch which fixes warnings about values that are assigned to variables, but never used.

          This is typically code where the declaration of a variable happens before it is assigned its intended value, and the declaration temporarily assigns a value that indicates it's unused (like null, 0, -1). It is better to leave the variables uninitialized in this case, as then the compiler will be able to detect and tell us if there are code paths where it's possible that the variable is not initialized before it's used.

          The patch cleans up the warnings by removing the unused assignments and, if possible, moving the declaration to where the first real assignment happens.

          All tests ran cleanly. Committed revision 1359624.

          Show
          Knut Anders Hatlen added a comment - Attaching d5847-4a-unused-assignment.patch which fixes warnings about values that are assigned to variables, but never used. This is typically code where the declaration of a variable happens before it is assigned its intended value, and the declaration temporarily assigns a value that indicates it's unused (like null, 0, -1). It is better to leave the variables uninitialized in this case, as then the compiler will be able to detect and tell us if there are code paths where it's possible that the variable is not initialized before it's used. The patch cleans up the warnings by removing the unused assignments and, if possible, moving the declaration to where the first real assignment happens. All tests ran cleanly. Committed revision 1359624.
          Hide
          Knut Anders Hatlen added a comment -

          Committed the 3a patch with revision 1359559.

          Show
          Knut Anders Hatlen added a comment - Committed the 3a patch with revision 1359559.
          Knut Anders Hatlen made changes -
          Hide
          Knut Anders Hatlen added a comment -

          Attaching d5847-3a-static-fields-and-imports.patch which fixes the references to static fields and organizes the imports alphabetically. This is also a partial fix for DERBY-436 (partial since it only touches DRDAConnThread, whereas that issue is for all the DRDA classes).

          Somewhat related, but not actually addressing any warnings, the patch also replaces references to fields in JDBC30Translation with java.sql.ParameterMetaData, as the ParameterMetaData interface has been available on the minimum compiler level for the DRDA code for quite a while now.

          Regression tests ran cleanly with the patch.

          Show
          Knut Anders Hatlen added a comment - Attaching d5847-3a-static-fields-and-imports.patch which fixes the references to static fields and organizes the imports alphabetically. This is also a partial fix for DERBY-436 (partial since it only touches DRDAConnThread, whereas that issue is for all the DRDA classes). Somewhat related, but not actually addressing any warnings, the patch also replaces references to fields in JDBC30Translation with java.sql.ParameterMetaData, as the ParameterMetaData interface has been available on the minimum compiler level for the DRDA code for quite a while now. Regression tests ran cleanly with the patch.
          Knut Anders Hatlen made changes -
          Attachment d5847-2a-unnecessary-return.patch [ 12535659 ]
          Hide
          Knut Anders Hatlen added a comment -

          Attaching d5847-2a-unnecessary-return.patch which cleans up another batch of warnings. The patch removes return statements at the end of void methods, as they are not needed.

          Committed revision 1359110.

          Show
          Knut Anders Hatlen added a comment - Attaching d5847-2a-unnecessary-return.patch which cleans up another batch of warnings. The patch removes return statements at the end of void methods, as they are not needed. Committed revision 1359110.
          Hide
          Knut Anders Hatlen added a comment -

          Committed the 1a patch to trunk with revision 1359098.

          Show
          Knut Anders Hatlen added a comment - Committed the 1a patch to trunk with revision 1359098.
          Hide
          Knut Anders Hatlen added a comment -

          Turns out this is a moving target. Just upgraded to NetBeans 7.2, and then the number of warnings in that file increased to 434. I disabled the warnings for missing curly braces in if/else/for/while statements, and then it went down to 71, which is more manageable.

          Show
          Knut Anders Hatlen added a comment - Turns out this is a moving target. Just upgraded to NetBeans 7.2, and then the number of warnings in that file increased to 434. I disabled the warnings for missing curly braces in if/else/for/while statements, and then it went down to 71, which is more manageable.
          Hide
          Kathey Marsden added a comment -

          linking DERBY-436 which was filed for the static field references and imports in all of the DRDA classes. These two issues intersect wrt DRDAConnThread.

          Show
          Kathey Marsden added a comment - linking DERBY-436 which was filed for the static field references and imports in all of the DRDA classes. These two issues intersect wrt DRDAConnThread.
          Kathey Marsden made changes -
          Link This issue is related to DERBY-436 [ DERBY-436 ]
          Knut Anders Hatlen made changes -
          Status Open [ 1 ] In Progress [ 3 ]
          Knut Anders Hatlen made changes -
          Field Original Value New Value
          Attachment d5847-1a-string-equality.patch [ 12535221 ]
          Hide
          Knut Anders Hatlen added a comment -

          Attaching a first patch, d5847-1a-string-equality.patch, that makes writeSQLDIAGGRP() use String.equals() instead of != to compare SQL states. It also removes an unused sqlState variable. All tests passed with the patch.

          Show
          Knut Anders Hatlen added a comment - Attaching a first patch, d5847-1a-string-equality.patch, that makes writeSQLDIAGGRP() use String.equals() instead of != to compare SQL states. It also removes an unused sqlState variable. All tests passed with the patch.
          Knut Anders Hatlen created issue -

            People

            • Assignee:
              Knut Anders Hatlen
              Reporter:
              Knut Anders Hatlen
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development