Community
Participate
Working Groups
3.7 M2. Due to a bad fix for bug 316937, the raw parameter names are put into the cache. This means that if the first attempt to read the parameter from e.g. Javadoc over the net times out, I end up with the raw names in the cache and even if the parameters could be fetched later, I get the raw names back. Test Case: 1. set timeout for fetching parameter names to 5ms 2. JLabel l; l.<code assist> ==> proposals show raw names 3. set timeout for fetching parameter names to 5000ms 4. JLabel l; l.<code assist> ==> proposals still show raw names EXPECTED: proposals show the parameter names fetched from Javadoc
Jay, this must be fixed for M4. Thanks.
Created attachment 183576 [details] Proposed patch Though, in the current thread, raw values are used for parameter names, the Javadoc is fetched anyway and cached further in the new thread and will be available next time on. So, no need to cache the raw names at the end of the method. I will see if I can add a regression test.
(In reply to comment #2) > I will see if I can add a regression test. Since it's a timing issue, I don't know how reliable a test can be. I guess I will let this patch go in as is.
(In reply to comment #3) > (In reply to comment #2) > > I will see if I can add a regression test. > > Since it's a timing issue, I don't know how reliable a test can be. I guess I > will let this patch go in as is. A test should be easy to write: just craft it along the lines outlined in comment 0.
Created attachment 183820 [details] Patch with test Same patch with test. Had to introduce a time delay in the test so that the previous attempt to load the Javadoc gets enough time, even though the code complete would have returned raw parameter names.
Created attachment 183821 [details] Binary and javadoc for the new test This archive contains the JAR with classes and a zip with the Javadocs that are required for the new test. Both the JAR and javadoc zip go inside "/org.eclipse.jdt.core.tests.model/workspace/Completion"
> Had to introduce a time delay in the test so that the previous attempt to load > the Javadoc gets enough time, even though the code complete would have > returned raw parameter names. Couldn't you use/set the preference like I did in comment 0?
(In reply to comment #7) > Couldn't you use/set the preference like I did in comment 0? There's a problem there. When we do it manually, there is enough delay between successive attempts and by the time we make the second attempt, the Javadoc has already been loaded. However, in the test, the second attempt is too quick that it fetches BinaryType.EMPTY_JAVADOC for Javadoc in BinaryMethod (line no 205) and hence condition if (javadocContents == null) is not satisfied. And I didn't want to touch a working code and it won't matter in the IDE. Actually, since the Javadoc contents are cached, setting the delay preference to a higher value doesn't mean much.
(In reply to comment #8) > (In reply to comment #7) > > Couldn't you use/set the preference like I did in comment 0? > > There's a problem there. When we do it manually, there is enough delay between > successive attempts and by the time we make the second attempt, the Javadoc has > already been loaded. Agreed. > Actually, since the Javadoc contents are cached, setting the delay preference > to a higher value doesn't mean much. Well, actually the test should show that if the first attempt didn't fetch the parameters, a second attempt with a higher timeout would do the trick.
Created attachment 183835 [details] Patch with updated test I guess the test now looks closer to the bug description. But it required a perProjectInfo cache flush (not the BinaryMethod cache)
The fix looks good, but the regression test fails consistently on my machine at least inside the IDE. The first test: assertResults("foo[METHOD_REF]{foo(), Lbug.X;, (Ljava.lang.Object;)V, foo, (arg0), 35}", requestor.getResults()); always returns an empty string. So this test should not be released. I don't see how a reliable test can be added for this. It will always be dependent of the time needed to retrieve the parameter names. I tried to reproduce the problem in comment 0, and it always works (with or without the fix). Daniel, you should try to reproduce the problem with the fix. If it works fine, then the fix should be released.
> I tried to reproduce the problem in comment 0, and it always works (with or > without the fix). That sounds strange. Are you sure you removed the source before trying this?
(In reply to comment #12) > That sounds strange. Are you sure you removed the source before trying this? You are right. Removing the source attachment reproduces the problem. The fix works fine. Only the regression test looks unreliable. So we should fix this for M4 and work on the regression test. This test needs to be reliable otherwise it will fail randomly during Eclipse builds. Jay, please make sure that the fix is released for M4. Thanks.
Created attachment 184103 [details] Patch with updated test I have changed following things in this test. 1. Instead of using content assist, the test directly calls/tests IMethod.getParameterNames(). So, one less factor to affect the test. 2. I have introduced a time delay that looks for the cached javadoc contents before flushing it. This eliminated the chance of the cache being populated by the previous attempt after the flush. The delay at most is 5000ms and if the javadoc content is not read by then, the test is likely to fail. 3. Moved the test to AttachedJavadocTests. Despite the new changes, the test can still fail if the javadoc 'read' is successful in 1ms or doesn't complete within 5000ms. Hypothetically, i.e. And we won't be able to fix those cases, simply because of the way the code has been written in BinaryMethod. But I guess we don't want to change the fix itself, do we?
Comment on attachment 184103 [details] Patch with updated test The fix is not good: it misses the case when the timeout is set to 0 ms.
> Despite the new changes, the test can still fail if the javadoc 'read' is > successful in 1ms That part can be avoided by setting it to 0 (once the patch is correct ;-). > we won't be able to fix those cases, simply because of the way the code has > been written in BinaryMethod. But I guess we don't want to change the fix > itself, do we? Correct, we shouldn't tweak the production code for that.
Created attachment 184122 [details] Proposed fix This is just the patch for BinaryMethod not to cache the parameter names when the timeout value is 0. In this case we simply return the raw names, but we don't cache the value so that if the timeout is changed, the parameter names will be recomputed. Daniel, do you agree with this change?
Created attachment 184129 [details] Same patch with updated test Same patch that Olivier posted plus the test to use '0' as the timeout. Now the test doesn't test the scenario where a very low but non-zero timeout was used. But that was not reliable, anyway. I am still worried about the possibility of the test failing when the javadoc file reading takes longer than 5 secs.
+1. This time the test passed. Jay, please release.
Released in HEAD for 3.7 M4.
>Daniel, do you agree with this change? Yep.
Verified for 3.7M4 using build id I20101205-2000
