You are viewing a plain text version of this content. The canonical link for it is here.
Posted to derby-dev@db.apache.org by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org> on 2007/11/11 22:54:50 UTC
[jira] Created: (DERBY-3198) Using setQueryTimeout will leak
sections
Using setQueryTimeout will leak sections
-----------------------------------------
Key: DERBY-3198
URL: https://issues.apache.org/jira/browse/DERBY-3198
Project: Derby
Issue Type: Bug
Components: JDBC, Network Client
Affects Versions: 10.3.1.4
Reporter: Dyre Tjeldvoll
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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Dyre Tjeldvoll updated DERBY-3198:
----------------------------------
Attachment: derby-3198.v2.diff
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)
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Attachments: derby-3198.v1.diff, derby-3198.v2.diff, repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Knut Anders Hatlen (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12548655 ]
Knut Anders Hatlen commented on DERBY-3198:
-------------------------------------------
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.
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Attachments: derby-3198.v1.diff, derby-3198.v2.diff, derby-3198.v3.diff, derby-3198.v4.diff, repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Dyre Tjeldvoll updated DERBY-3198:
----------------------------------
Derby Info: (was: [Patch Available])
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.
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Attachments: derby-3198.v1.diff, derby-3198.v2.diff, repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12545415 ]
Dyre Tjeldvoll commented on DERBY-3198:
---------------------------------------
I forgot to mention that derbyall and suites.All (including the new test case for this issue which fails on trunk) both pass.
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Attachments: derby-3198.v1.diff, repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12546665 ]
Dyre Tjeldvoll commented on DERBY-3198:
---------------------------------------
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.
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Attachments: derby-3198.v1.diff, derby-3198.v2.diff, repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Dyre Tjeldvoll updated DERBY-3198:
----------------------------------
Attachment: derby-3198.v3.diff
New patch (v3) with dedicated Section for writeSetSpeialRegister() which is freed in Statement.markClosed()
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Attachments: derby-3198.v1.diff, derby-3198.v2.diff, derby-3198.v3.diff, repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Dyre Tjeldvoll updated DERBY-3198:
----------------------------------
Attachment: derby-3198.v4.diff
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.
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Attachments: derby-3198.v1.diff, derby-3198.v2.diff, derby-3198.v3.diff, derby-3198.v4.diff, repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Knut Anders Hatlen (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12545874 ]
Knut Anders Hatlen commented on DERBY-3198:
-------------------------------------------
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 :)
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Attachments: derby-3198.v1.diff, derby-3198.v2.diff, repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Dyre Tjeldvoll updated DERBY-3198:
----------------------------------
Derby Info: (was: [Patch Available])
Committed revision 602495.
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Attachments: derby-3198.v1.diff, derby-3198.v2.diff, derby-3198.v3.diff, derby-3198.v4.diff, derby-3198.v5.diff, derby-3198.v6.diff, repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12547837 ]
Dyre Tjeldvoll commented on DERBY-3198:
---------------------------------------
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.
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Attachments: derby-3198.v1.diff, derby-3198.v2.diff, derby-3198.v3.diff, repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Dyre Tjeldvoll updated DERBY-3198:
----------------------------------
Derby Info: (was: [Patch Available])
I'm making some more changes, and I'll make another version of the patch.
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Attachments: derby-3198.v1.diff, derby-3198.v2.diff, derby-3198.v3.diff, repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Dyre Tjeldvoll updated DERBY-3198:
----------------------------------
Derby Info: [Patch Available]
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Attachments: derby-3198.v1.diff, derby-3198.v2.diff, derby-3198.v3.diff, repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Knut Anders Hatlen (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12548959 ]
Knut Anders Hatlen commented on DERBY-3198:
-------------------------------------------
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.)
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Attachments: derby-3198.v1.diff, derby-3198.v2.diff, derby-3198.v3.diff, derby-3198.v4.diff, derby-3198.v5.diff, repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12542160 ]
Dyre Tjeldvoll commented on DERBY-3198:
---------------------------------------
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
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Attachments: repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Dyre Tjeldvoll updated DERBY-3198:
----------------------------------
Attachment: derby-3198.v6.diff
v6 is the final (?) version of this patch. All tests pass. Plan to commit shortly.
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Attachments: derby-3198.v1.diff, derby-3198.v2.diff, derby-3198.v3.diff, derby-3198.v4.diff, derby-3198.v5.diff, derby-3198.v6.diff, repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Myrna van Lunteren (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12541887 ]
Myrna van Lunteren commented on DERBY-3198:
-------------------------------------------
I wonder if this could be related to the trouble we sometimes see with specific jvms with the setQueryTimeoutTest, such as DERBY-1848.
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Attachments: repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Dyre Tjeldvoll updated DERBY-3198:
----------------------------------
Attachment: repro.diff
Attaching a patch which modifies derbyStress.java so that the problem is exposed.
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Attachments: repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Dyre Tjeldvoll updated DERBY-3198:
----------------------------------
Attachment: derby-3198.v1.diff
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.
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Attachments: derby-3198.v1.diff, repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12544883 ]
Dyre Tjeldvoll commented on DERBY-3198:
---------------------------------------
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.
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Attachments: repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Dyre Tjeldvoll updated DERBY-3198:
----------------------------------
Derby Info: [Patch Available]
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Attachments: derby-3198.v1.diff, repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Dyre Tjeldvoll updated DERBY-3198:
----------------------------------
Derby Info: [Patch Available]
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Attachments: derby-3198.v1.diff, derby-3198.v2.diff, derby-3198.v3.diff, derby-3198.v4.diff, repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Knut Anders Hatlen (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12546724 ]
Knut Anders Hatlen commented on DERBY-3198:
-------------------------------------------
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();
}
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Attachments: derby-3198.v1.diff, derby-3198.v2.diff, repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Closed: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Dyre Tjeldvoll closed DERBY-3198.
---------------------------------
Fixed
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Attachments: derby-3198.v1.diff, derby-3198.v2.diff, derby-3198.v3.diff, derby-3198.v4.diff, derby-3198.v5.diff, derby-3198.v6.diff, repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12546734 ]
Dyre Tjeldvoll commented on DERBY-3198:
---------------------------------------
Quite right :) (And I can confirm that tests fail if you don't free the Section when you close the Statement :)
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Attachments: derby-3198.v1.diff, derby-3198.v2.diff, repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Resolved: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Dyre Tjeldvoll resolved DERBY-3198.
-----------------------------------
Resolution: Fixed
Committed revision 602495.
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Attachments: derby-3198.v1.diff, derby-3198.v2.diff, derby-3198.v3.diff, derby-3198.v4.diff, derby-3198.v5.diff, derby-3198.v6.diff, repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Assigned: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Dyre Tjeldvoll reassigned DERBY-3198:
-------------------------------------
Assignee: Dyre Tjeldvoll
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Attachments: derby-3198.v1.diff, repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12549027 ]
Dyre Tjeldvoll commented on DERBY-3198:
---------------------------------------
>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.
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Attachments: derby-3198.v1.diff, derby-3198.v2.diff, derby-3198.v3.diff, derby-3198.v4.diff, derby-3198.v5.diff, repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Dyre Tjeldvoll updated DERBY-3198:
----------------------------------
Fix Version/s: 10.4.0.0
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).
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Fix For: 10.4.0.0
>
> Attachments: derby-3198.v1.diff, derby-3198.v2.diff, derby-3198.v3.diff, derby-3198.v4.diff, derby-3198.v5.diff, derby-3198.v6.diff, repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12543087 ]
Dyre Tjeldvoll commented on DERBY-3198:
---------------------------------------
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.
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Attachments: repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Knut Anders Hatlen (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12547827 ]
Knut Anders Hatlen commented on DERBY-3198:
-------------------------------------------
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.
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Attachments: derby-3198.v1.diff, derby-3198.v2.diff, derby-3198.v3.diff, repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (DERBY-3198) Using setQueryTimeout will leak
sections
Posted by "Dyre Tjeldvoll (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/DERBY-3198?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Dyre Tjeldvoll updated DERBY-3198:
----------------------------------
Attachment: derby-3198.v5.diff
Attaching v5 to address the latest comments.
> Using setQueryTimeout will leak sections
> -----------------------------------------
>
> Key: DERBY-3198
> URL: https://issues.apache.org/jira/browse/DERBY-3198
> Project: Derby
> Issue Type: Bug
> Components: JDBC, Network Client
> Affects Versions: 10.3.1.4
> Reporter: Dyre Tjeldvoll
> Assignee: Dyre Tjeldvoll
> Attachments: derby-3198.v1.diff, derby-3198.v2.diff, derby-3198.v3.diff, derby-3198.v4.diff, derby-3198.v5.diff, repro.diff
>
>
> 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.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.