Dyre Tjeldvoll (JIRA) wrote:
AB, your comment about the patch almost hit the nail on the head, but not quite.
The CliParam struct has field called isExt that indicates that the parameter is
external (in its own DSS). Just like the drdaType and the drdaLength filelds can
change (to my surprise), the isExt can go from true to false for the same parameter
in the same statement. In this case param 7 had been marked as external, but was
internal in this message. Hence the attempt to read the separate DSS left the
server waiting for more data, data which the client was never going to send.
Thanks for looking into this regression. I (AB) applied the patch and ran my
ODBC tests again, and the hang I reported is now gone. That said, I'm now
seeing a different, intermittent hang in one of the multi-threaded tests that I
have (again for ODBC). I can't say for sure if this intermittent hang is the
result of the DERBY-815 changes or just a problem with the test--I'm still
trying to figure that out. While I investigate the matter, I'm wondering if, by
chance, you can think of anything in your DERBY-815 patch that might be
affecting the server when running with a multi-threaded client? I'd be grateful
for any ideas or suggestions that you might have there...
Regarding your proposed fix for the regression:
With the following change your repro goes through:
final DRDAStatement.CliParam cp =
stmt.getCliParam(j);
+ cp.isExt = false; // <--- FIX
+
if (cp.drdaType != drdaType ||
cp.drdaLength != drdaLength) {
if (SanityManager.DEBUG) {
I'll polish up a proper patch and attach it when I've run derbyall. Sorry about
all the problems...
Three things regarding the d815_regress.diff patch that you posted:
1) The need for this fix is apparently non-intuitive--you yourself mentioned
that you were "surprised" by that fact that "isExt can go from true to false for
the same parameter in the same statement". So it'd be nice if you could add
comments to the code indicating _why_ this particular line (cp.isExt = false) is
required, and _when_ such a scenario can happen. If you were surprised, you can
bet others will be, too :) Three or four years from now, when you or I or
someone who's never looked at this code before sees this line, they could very
well go "Ummm...what's that for?" and then subsequently delete it, or maybe even
do worse--and the derbyall tests wouldn't catch it, and thus the regression
would yet again be reintroduced. Which leads me to #2...
2) It would be great if you could take the repro for the regression and
incorporate it into one of the tests in the derbyall suite, so that it runs
every night. That way, if someone _does_ go through and delete the above line
from the code, the nightlies will catch it. It's always desirable to have a
test case submitted with code changes; the original patch for DERBY-815 was sort
of an exception, since the theory was that it wasn't changing/adding any
functionality and thus existing tests would be sufficient. But for the
regression, we have a definite test case for "broken" and "fixed", so I think we
should have that test case as part of the nightlies.
3) Okay, so it looks like my guess regarding the following code was wrong:
+ // Trust that this is OK...
+ cp.drdaType = drdaType;
+ cp.drdaLength = drdaLength;
That said, I have to admit that I'm uncomfortable with "Trust that this is
OK..." as a comment in a final patch. The comment itself makes it seem like
this is a bit of guess-work coding--sort of a "well I don't know why we need to
do this, but let's try it and cross our fingers" thing. I'm assuming that
that's _not_ the case, so it would be extremely helpful to reword the comment to
explain _why_ we need to do this assignment. When I ran into the regression and
started looking at the patch, I sat there and thought "Okay, what does that
mean? Why was this code added? What happens if I change it? What am I
'trusting' in? What did the developer have in mind when he added this code?"
And that's why I pointed it out in my comment when I posted the repro. So at
the very least, it's a comment that can cause developers to spend time
scratching their heads and wondering whether or not this particular code might
be the cause of some server problem that they're having (like, say, a hang with
an ODBC client... ;)
Thanks again for addressing this regression so quickly. I don't mean to
disregard your efforts nor your fix--I appreciate both! I just think it might
be helpful for future developers if the new code contained some additional
explanatory comments...
Army
