Derby
  1. Derby
  2. DERBY-3198

Using setQueryTimeout will leak sections

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.3.1.4
    • Fix Version/s: 10.4.1.3
    • Component/s: JDBC, Network Client
    • Labels:
      None

      Description

      The implementation of setQueryTimeout relies on NetStatementReply.writeSetSpecialRegister() which will allocate a dynamic section when called. No reference to this Section object is kept, and so Section.free() never gets called on it. Executing the same statment repeatedly with a query timeout set results in the client driver throwing an exception because the number of Sections exceeding 32000.

      1. repro.diff
        0.7 kB
        Dyre Tjeldvoll
      2. derby-3198.v6.diff
        8 kB
        Dyre Tjeldvoll
      3. derby-3198.v5.diff
        8 kB
        Dyre Tjeldvoll
      4. derby-3198.v4.diff
        7 kB
        Dyre Tjeldvoll
      5. derby-3198.v3.diff
        5 kB
        Dyre Tjeldvoll
      6. derby-3198.v2.diff
        5 kB
        Dyre Tjeldvoll
      7. derby-3198.v1.diff
        5 kB
        Dyre Tjeldvoll

        Issue Links

          Activity

          Hide
          Dyre Tjeldvoll added a comment -

          This issue has no fix version, but has been checked in to trunk after the 10.3 branch was cut, and has not been merged to the 10.3 branch (before the last release).
          (Some have been checked into trunk after 10.4 was cut and then merged to 10.4).

          Show
          Dyre Tjeldvoll added a comment - This issue has no fix version, but has been checked in to trunk after the 10.3 branch was cut, and has not been merged to the 10.3 branch (before the last release). (Some have been checked into trunk after 10.4 was cut and then merged to 10.4).
          Hide
          Dyre Tjeldvoll added a comment -

          Fixed

          Show
          Dyre Tjeldvoll added a comment - Fixed
          Hide
          Dyre Tjeldvoll added a comment -

          Committed revision 602495.

          Show
          Dyre Tjeldvoll added a comment - Committed revision 602495.
          Hide
          Dyre Tjeldvoll added a comment -

          Committed revision 602495.

          Show
          Dyre Tjeldvoll added a comment - Committed revision 602495.
          Hide
          Dyre Tjeldvoll added a comment -

          v6 is the final version of this patch. All tests pass. Plan to commit shortly.

          Show
          Dyre Tjeldvoll added a comment - v6 is the final version of this patch. All tests pass. Plan to commit shortly.
          Hide
          Dyre Tjeldvoll added a comment -

          From what I can tell, the call to Section.free() in Statement.initResetStatement() can either come from a call to Statement's constructor, or from a chain of 13 method calls originating from ClientPooledConnection.reset().

          My interpretation of this is that by freeing Sections in initResetStatement() we ensure that Statements belonging to unused Connections in a connection pool do not hold on to Section objects. This sounds reasonable, so I think it is a good idea to free setSpecialRegisterSection_ in Statement.initResetStatement() as well.

          Show
          Dyre Tjeldvoll added a comment - From what I can tell, the call to Section.free() in Statement.initResetStatement() can either come from a call to Statement's constructor, or from a chain of 13 method calls originating from ClientPooledConnection.reset(). My interpretation of this is that by freeing Sections in initResetStatement() we ensure that Statements belonging to unused Connections in a connection pool do not hold on to Section objects. This sounds reasonable, so I think it is a good idea to free setSpecialRegisterSection_ in Statement.initResetStatement() as well.
          Hide
          Knut Anders Hatlen added a comment -

          Thanks for addressing my comments, Dyre. The tests look good, and the fix looks correct and clear. So I think the patch should be committed.

          By the way, Statement.reset() says something about changing the section. I'm not sure if it applies to the section used by writeSetSpecialRegister(), but you may want to take a look at it just in case. (I noticed that section_.free() is called more places in the code than setSpecialRegisterSection_.free(). I think those calls are either related to batching or to reset(), so I don't think it's important for the special register section, but I wanted to mention it just in case.)

          Show
          Knut Anders Hatlen added a comment - Thanks for addressing my comments, Dyre. The tests look good, and the fix looks correct and clear. So I think the patch should be committed. By the way, Statement.reset() says something about changing the section. I'm not sure if it applies to the section used by writeSetSpecialRegister(), but you may want to take a look at it just in case. (I noticed that section_.free() is called more places in the code than setSpecialRegisterSection_.free(). I think those calls are either related to batching or to reset(), so I don't think it's important for the special register section, but I wanted to mention it just in case.)
          Hide
          Dyre Tjeldvoll added a comment -

          Attaching v5 to address the latest comments.

          Show
          Dyre Tjeldvoll added a comment - Attaching v5 to address the latest comments.
          Hide
          Knut Anders Hatlen added a comment -

          The new test case looks fine. You might consider if you want to change the assert for i==16383 to i>=16383, since it's not technically an error if more than 16383 statements can be open at the same time. Also, since the test now uses the string "StatementJdbc30Test:client" twice, and won't add the new test cases unless both occurrences have the same spelling, perhaps you should define a constant to ensure their consistency.

          Show
          Knut Anders Hatlen added a comment - The new test case looks fine. You might consider if you want to change the assert for i==16383 to i>=16383, since it's not technically an error if more than 16383 statements can be open at the same time. Also, since the test now uses the string "StatementJdbc30Test:client" twice, and won't add the new test cases unless both occurrences have the same spelling, perhaps you should define a constant to ensure their consistency.
          Hide
          Dyre Tjeldvoll added a comment -

          Reading Knut's comment again I realized that he was talking about closing ResultSets and not Statements. I agree that it is no reason not to do this. Relying on implicit close does not alter the use/freeing of Sections.

          The comments also made me want to test what happens when a large number of Statements are kept open, since Statements with query timeout will hold on to two Sections (as opposed to only one for Statements without query timeout). So I added a test case for that.

          Show
          Dyre Tjeldvoll added a comment - Reading Knut's comment again I realized that he was talking about closing ResultSets and not Statements. I agree that it is no reason not to do this. Relying on implicit close does not alter the use/freeing of Sections. The comments also made me want to test what happens when a large number of Statements are kept open, since Statements with query timeout will hold on to two Sections (as opposed to only one for Statements without query timeout). So I added a test case for that.
          Hide
          Dyre Tjeldvoll added a comment -

          I'm making some more changes, and I'll make another version of the patch.

          Show
          Dyre Tjeldvoll added a comment - I'm making some more changes, and I'll make another version of the patch.
          Hide
          Dyre Tjeldvoll added a comment -

          Well, it seemed safer to verify that Sections aren't leaked even when relying on implicit close. I could add a comment about that I suppose.

          Show
          Dyre Tjeldvoll added a comment - Well, it seemed safer to verify that Sections aren't leaked even when relying on implicit close. I could add a comment about that I suppose.
          Hide
          Knut Anders Hatlen added a comment -

          Patch looks good to me. +1 to commit.

          You might also consider closing the result set in the test, unless it's supposed to test that the section is freed on implicit close.

          Show
          Knut Anders Hatlen added a comment - Patch looks good to me. +1 to commit. You might also consider closing the result set in the test, unless it's supposed to test that the section is freed on implicit close.
          Hide
          Dyre Tjeldvoll added a comment -

          New patch (v3) with dedicated Section for writeSetSpeialRegister() which is freed in Statement.markClosed()

          Show
          Dyre Tjeldvoll added a comment - New patch (v3) with dedicated Section for writeSetSpeialRegister() which is freed in Statement.markClosed()
          Hide
          Dyre Tjeldvoll added a comment -

          Quite right (And I can confirm that tests fail if you don't free the Section when you close the Statement

          Show
          Dyre Tjeldvoll added a comment - Quite right (And I can confirm that tests fail if you don't free the Section when you close the Statement
          Hide
          Knut Anders Hatlen added a comment -

          Reusing it instead of creating a new one on each execution sounds reasonable to me. But don't we still need some clean-up code when we close the statement? Otherwise, this code will leak sections, won't it?

          Connection c = DriverManager.getConnection("jdbc:derby://localhost/db");
          while (true)

          { Statement s = c.createStatement(); s.setQueryTimeout(100); s.executeQuery("values 1").close(); s.close(); }
          Show
          Knut Anders Hatlen added a comment - Reusing it instead of creating a new one on each execution sounds reasonable to me. But don't we still need some clean-up code when we close the statement? Otherwise, this code will leak sections, won't it? Connection c = DriverManager.getConnection("jdbc:derby://localhost/db"); while (true) { Statement s = c.createStatement(); s.setQueryTimeout(100); s.executeQuery("values 1").close(); s.close(); }
          Hide
          Dyre Tjeldvoll added a comment -

          If we go for the separate member variable option; do we really need to free this Section? If/when we start using this to cache session data, that Section
          will be needed for every message, so requesting it and freeing it each time is just going to be extra work, isn't it? Or would that have some other drawback that I'm not seeing? I'm running the tests with this change now, to see how it works.

          Show
          Dyre Tjeldvoll added a comment - If we go for the separate member variable option; do we really need to free this Section? If/when we start using this to cache session data, that Section will be needed for every message, so requesting it and freeing it each time is just going to be extra work, isn't it? Or would that have some other drawback that I'm not seeing? I'm running the tests with this change now, to see how it works.
          Hide
          Dyre Tjeldvoll added a comment -

          Hi Knut,
          Thanks for taking the time to look at the patch. I'll try to answer your questions:

          Q: Will there ever be a case when section_ is null so that we need to generate a new one?
          A: Yes, if you don't check for null, you'll get an NPE.

          Q: And if we generate a new one, will that one be reused later?
          A: That depends on where setSpecialRegister is called. Sometimes it will be reused, but other times a new Section will be allocated regardless.

          Q: or will a new one be allocated and section_ replaced without
          freeing the old one?
          A: No, I actually don't think that will
          happen. As far as I can tell, the only place where section_ is
          assigned without verifying that it is null or that the
          previous value has been freed, is in Statement's constructor and init methods.

          Q: Should we instead have used the holdability of the statement,
          since this section can now be used for other statements than the
          set special register statement?
          A: Possibly. But my impression is that the Section stored in the
          section_ variable is reused only when you don't care about the
          type of Section. In the case where the holdability of the Section
          actually matters, a separate Section, with the proper
          holdability, is kept in a local variable (newSection), and this
          variable is transfered to the section_ variable after its
          previous value is freed. I could be wrong about this, though.

          I like your idea about using a separate member variable for
          tracking the Section used in setSpecialRegister and I agree that
          it more closely mimics the old behavior. The only reason for not
          choosing that option from the beginning was that I did not see
          any other method that did its own Section management like that.

          Show
          Dyre Tjeldvoll added a comment - Hi Knut, Thanks for taking the time to look at the patch. I'll try to answer your questions: Q: Will there ever be a case when section_ is null so that we need to generate a new one? A: Yes, if you don't check for null, you'll get an NPE. Q: And if we generate a new one, will that one be reused later? A: That depends on where setSpecialRegister is called. Sometimes it will be reused, but other times a new Section will be allocated regardless. Q: or will a new one be allocated and section_ replaced without freeing the old one? A: No, I actually don't think that will happen. As far as I can tell, the only place where section_ is assigned without verifying that it is null or that the previous value has been freed, is in Statement's constructor and init methods. Q: Should we instead have used the holdability of the statement, since this section can now be used for other statements than the set special register statement? A: Possibly. But my impression is that the Section stored in the section_ variable is reused only when you don't care about the type of Section. In the case where the holdability of the Section actually matters, a separate Section, with the proper holdability, is kept in a local variable (newSection), and this variable is transfered to the section_ variable after its previous value is freed. I could be wrong about this, though. I like your idea about using a separate member variable for tracking the Section used in setSpecialRegister and I agree that it more closely mimics the old behavior. The only reason for not choosing that option from the beginning was that I did not see any other method that did its own Section management like that.
          Hide
          Knut Anders Hatlen added a comment -

          Hi Dyre,

          Your patch is probably OK, but there are two things that are not quite clear to me:

          1) Will there ever be a case when section_ is null so that we need to generate a new one? And if we generate a new one, will that one be reused later, or will a new one be allocated and section_ replaced without freeing the old one?

          2) When a section is created on the fly, its holdability is set to HOLD_CURSORS_OVER_COMMIT. Should we instead have used the holdability of the statement, since this section can now be used for other statements than the set special register statement?

          If Statement instead had a field called specialRegisterSection which, if not null, was freed at the same time as section_ was freed, I think it would be easier to understand the fix, and the behaviour would be closer to the old one (with the exception of the leak, of course

          Show
          Knut Anders Hatlen added a comment - Hi Dyre, Your patch is probably OK, but there are two things that are not quite clear to me: 1) Will there ever be a case when section_ is null so that we need to generate a new one? And if we generate a new one, will that one be reused later, or will a new one be allocated and section_ replaced without freeing the old one? 2) When a section is created on the fly, its holdability is set to HOLD_CURSORS_OVER_COMMIT. Should we instead have used the holdability of the statement, since this section can now be used for other statements than the set special register statement? If Statement instead had a field called specialRegisterSection which, if not null, was freed at the same time as section_ was freed, I think it would be easier to understand the fix, and the behaviour would be closer to the old one (with the exception of the leak, of course
          Hide
          Dyre Tjeldvoll added a comment -

          Attaching *.v2.diff which replaces the previous patch. The first version contained an extra test case which I was experimenting with, but which does not belong in that test, and should not be committed. (I plan to commit this patch in a couple of days unless anyone advices differently)

          Show
          Dyre Tjeldvoll added a comment - Attaching *.v2.diff which replaces the previous patch. The first version contained an extra test case which I was experimenting with, but which does not belong in that test, and should not be committed. (I plan to commit this patch in a couple of days unless anyone advices differently)
          Hide
          Dyre Tjeldvoll added a comment -

          I forgot to mention that derbyall and suites.All (including the new test case for this issue which fails on trunk) both pass.

          Show
          Dyre Tjeldvoll added a comment - I forgot to mention that derbyall and suites.All (including the new test case for this issue which fails on trunk) both pass.
          Hide
          Dyre Tjeldvoll added a comment -

          Attaching a patch (*.v1.stat) which re-uses am.Statement.section_ if available, or assigns a newly allocated Section to this variable so that it can be freed.

          Show
          Dyre Tjeldvoll added a comment - Attaching a patch (*.v1.stat) which re-uses am.Statement.section_ if available, or assigns a newly allocated Section to this variable so that it can be freed.
          Hide
          Dyre Tjeldvoll added a comment -

          Based on what I see methods similar to writeSetSpecialRegister do, I think a safe and reasonable way of getting a Section is as follows:

          1) If the surrounding code has set the newSection variable, use that. This will usually be the case when writeSetSpecialRegister is used to request session data, as it happens after this variable has been assigned.

          2) If Statement.section_ is not null, it is probably safe to use it.

          3) if Statement.section_ is null, request a new one from the SectionManager and assign that to section_. Assigning it to section_ will ensure that it is freed later.

          Doing so does, however, require a change to the signature of the writeSetSpecialRegister() methods in the Net-layer, so that the Section to use can be passed in as a parameter (this seems to be how it is done in other write methods). The change is necessary because the decision about which Section to use need to happen in the am-layer. It cannot be done in NetStatementRequest as it currently is, because there we don't have access to the Section variables mentioned earlier.

          I'm wondering if the ability to access the SectionManager from the Net-layer is an unintended loophole. Seems like NetStatementReply.writeSetSpecialRegister() is the only place in the net package where it is used. The SectionManager is accessed through a public member in am.Agent which has the following comment:
          public SectionManager sectionManager_ = null; // temporarily public, make friendly at least !!

          The Net-layer gets at this member because NetAgent extends am.Agent.

          Show
          Dyre Tjeldvoll added a comment - Based on what I see methods similar to writeSetSpecialRegister do, I think a safe and reasonable way of getting a Section is as follows: 1) If the surrounding code has set the newSection variable, use that. This will usually be the case when writeSetSpecialRegister is used to request session data, as it happens after this variable has been assigned. 2) If Statement.section_ is not null, it is probably safe to use it. 3) if Statement.section_ is null, request a new one from the SectionManager and assign that to section_. Assigning it to section_ will ensure that it is freed later. Doing so does, however, require a change to the signature of the writeSetSpecialRegister() methods in the Net-layer, so that the Section to use can be passed in as a parameter (this seems to be how it is done in other write methods). The change is necessary because the decision about which Section to use need to happen in the am-layer. It cannot be done in NetStatementRequest as it currently is, because there we don't have access to the Section variables mentioned earlier. I'm wondering if the ability to access the SectionManager from the Net-layer is an unintended loophole. Seems like NetStatementReply.writeSetSpecialRegister() is the only place in the net package where it is used. The SectionManager is accessed through a public member in am.Agent which has the following comment: public SectionManager sectionManager_ = null; // temporarily public, make friendly at least !! The Net-layer gets at this member because NetAgent extends am.Agent.
          Hide
          Dyre Tjeldvoll added a comment -

          I have now had a chance to look at DERBY-1848, but I don't think these issues are directly related. The Section issue will only occur when using the client driver, and exhausting the number of Sections produces a specific error message and call stack which I don't see in DERBY-1848. Based on the diff shown there, it seems like a statement times out unexpectedly, which I guess could happen if you happen to get a full GC (or other delay) at the wrong time.

          Show
          Dyre Tjeldvoll added a comment - I have now had a chance to look at DERBY-1848 , but I don't think these issues are directly related. The Section issue will only occur when using the client driver, and exhausting the number of Sections produces a specific error message and call stack which I don't see in DERBY-1848 . Based on the diff shown there, it seems like a statement times out unexpectedly, which I guess could happen if you happen to get a full GC (or other delay) at the wrong time.
          Hide
          Dyre Tjeldvoll added a comment -

          I writing this in a mail because I keep getting 503 from jira.

          I have not looked at the issue you describe (since jira is down),
          but it sounds reasonable.

          I tried fixing this by letting
          NetStatementRequest.writeSetSpecialRegister() just wrap its code in a
          try - finally where the finally clause calls free() on the newly
          allocated Section object.

          This seems to fix the leak, but you also change the
          semantic. Previously setSpecialRegister() (actually EXCSETSTT) was put
          in its own Section that was not used by any other statement, but with
          this fix the next call to getDynamicSection() will likely reuse the
          section number used by writeSetSpecialRegister(). Whether this
          reperesents a problem or not, I don't know...

          I tried reading the rules for assigning Section numbers in the DRDA
          spec (Vol 1, section 7.15 pages 460-462), but that left me with more
          questions than answers. It does seem like the section number should be
          used to group statements that "belong together". If that is correct,
          then I guess setQueryTimeout SHOULD have the same section number as
          the statment that follows since they belong together...

          Any DRDA experts reading this should feel free
          to chime in...


          Dyre

          Show
          Dyre Tjeldvoll added a comment - I writing this in a mail because I keep getting 503 from jira. I have not looked at the issue you describe (since jira is down), but it sounds reasonable. I tried fixing this by letting NetStatementRequest.writeSetSpecialRegister() just wrap its code in a try - finally where the finally clause calls free() on the newly allocated Section object. This seems to fix the leak, but you also change the semantic. Previously setSpecialRegister() (actually EXCSETSTT) was put in its own Section that was not used by any other statement, but with this fix the next call to getDynamicSection() will likely reuse the section number used by writeSetSpecialRegister(). Whether this reperesents a problem or not, I don't know... I tried reading the rules for assigning Section numbers in the DRDA spec (Vol 1, section 7.15 pages 460-462), but that left me with more questions than answers. It does seem like the section number should be used to group statements that "belong together". If that is correct, then I guess setQueryTimeout SHOULD have the same section number as the statment that follows since they belong together... Any DRDA experts reading this should feel free to chime in... – Dyre
          Hide
          Myrna van Lunteren added a comment -

          I wonder if this could be related to the trouble we sometimes see with specific jvms with the setQueryTimeoutTest, such as DERBY-1848.

          Show
          Myrna van Lunteren added a comment - I wonder if this could be related to the trouble we sometimes see with specific jvms with the setQueryTimeoutTest, such as DERBY-1848 .
          Hide
          Dyre Tjeldvoll added a comment -

          Attaching a patch which modifies derbyStress.java so that the problem is exposed.

          Show
          Dyre Tjeldvoll added a comment - Attaching a patch which modifies derbyStress.java so that the problem is exposed.

            People

            • Assignee:
              Dyre Tjeldvoll
              Reporter:
              Dyre Tjeldvoll
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development