Bugzilla – Full Text Bug Listing
| Summary: | [hotbug] Next button disabled when choosing existing server host name from the new server wizard | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [WebTools] WTP ServerTools | Reporter: | Christine <jycli> | ||||||||||||||||
| Component: | wst.server | Assignee: | Elson Yuen <eyuen7> | ||||||||||||||||
| Status: | VERIFIED FIXED | QA Contact: | Elson Yuen <eyuen7> | ||||||||||||||||
| Severity: | normal | ||||||||||||||||||
| Priority: | P3 | CC: | alexandrina.ivanova, bakalsky, dimitar.giormov, eyuen7, kaloyan, stefan.dimov, vladimir.pavlov | ||||||||||||||||
| Version: | 3.2 | ||||||||||||||||||
| Target Milestone: | 3.4.2 | ||||||||||||||||||
| Hardware: | All | ||||||||||||||||||
| OS: | All | ||||||||||||||||||
| URL: | plan_draft_332 | ||||||||||||||||||
| See Also: |
https://git.eclipse.org/r/109050 https://git.eclipse.org/r/109135 |
||||||||||||||||||
| Whiteboard: | |||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Christine
Created attachment 198021 [details]
A patch to fix this problem
Created attachment 198053 [details]
v1.0
There is a problem with the proposed fix that it will causes regression on the validation. When the host name change even is being skipped, it doesn't triggers a validation to be run to verify the host name again. This means that if any validation error will gets wiped out.
I am submitting a new patch to address the problem.
Created attachment 198204 [details]
An updated patch
This updated patch will deal with the validation issue, since even if the host name is the same as before, handleHostnameChange will still be called.
Tests run:
The host name "localhost2" is in the history list
1. Typing "localhost2", ensure the Next button is enabled
2. Typing "l" to bring up the history list, selecting "localhost2" and ensuring the Next button is enabled
3. Typing "localhost2" to bring up the history list, selecting "localhost2" and ensuring the Next button is enabled (selecting from the list, would call the return from hostnameChanged pre-patch, which would skip validations)
4. Typing "localhost2", then typing in "localhost2" without any pauses, and ensuring the Next button is enabled (this would call return from hostnameChanged pre-patch, which would skip validations)
Probably this could be related with the problem, reported here: http://www.eclipse.org/forums/index.php/t/210655/ In essence, when you switch between two different server types, that have the same string in the "server's host name" field, applying a change in the visibility, leads to a disabled Next button. The new patch looks good and the test coverage is good as well. Changes released to 32M. Created attachment 198497 [details]
v1.1
Update patch with clean up on previous patch.
Changes released to 33M and HEAD. Hi,
The problem appears again but now its hardly reproducible.
When called
NewServerWizardFragment.checkHostAndServerType() line: 163
NewServerWizardFragment.isComplete() line: 152
TaskWizardPage.canFlipToNextPage() line: 78
WizardDialog.updateButtons() line: 1350
WizardDialog.
TaskWizardPage.canFlipToNextPage() ->
NewServerWizardFragment.isComplete() -> NewServerWizardFragment.checkHostAndServerType()
sometimes this returns false on line 165:
if (manualComp.isTimerRunning() || manualComp.isTimerScheduled()) {
return false;
}
but in the nex
Hi,
Sorry for the previous comment, here it is:
The problem appears again but now its hardly reproducible.
When called
NewServerWizardFragment.checkHostAndServerType() line: 163
NewServerWizardFragment.isComplete() line: 152
TaskWizardPage.canFlipToNextPage() line: 78
WizardDialog.updateButtons() line: 1350
sometimes this returns false on line 165:
if (manualComp.isTimerRunning() || manualComp.isTimerScheduled()) {
return false;
}
but in the next moment, when the timer is finished, the method returns true if reevaluated.
So to summarise the problem, sometimes the check for enabling the Next button (the provided stack) is performed before the timer in NewManualServerComposite is finished and this way the Next button remains disabled.
In the proposed patch I wait in NewServerWizardFragment.checkHostAndServerType() for this timer to finish instead of just return "false" and this way the problem's gone, but it would be better if there is a way the ActionListener of this timer to trigger the update of the buttons again in its actionPerformed method.
Created attachment 201721 [details]
Wait the Timer to finish instead return 'false'.
Reopening this bug as per comment 9. The attached patch in comment 10 might not be the perfect solution, but it is a good starting point. Elson can you comment on the patch? thanks, Dimitar The design of the timer is supposed to be the action code for handling the host name change will only be called after the timer has expired. If there are new hostname changes coming in before the timer expires, then it will restart the timer again. Once the timer is expired, it will execute the host name change action which eventually calls the NewServerWizardFragment.checkHostAndServerType() to check for the host name again. Are the steps that you can reproduce the problem different from the list of test scenarios that has been described on comment #3? Can you describe more on the detailed steps that you see the problem? The proposed solution is simply doing a time wait to allow a grace period of 2sec for the problem to resolve by itself. I would prefer to find the actual cause of the solution and find a solution that will really fix the problem. (In reply to comment #13) > The design of the timer is supposed to be the action code for handling the host > name change will only be called after the timer has expired. If there are new > hostname changes coming in before the timer expires, then it will restart the > timer again. Once the timer is expired, it will execute the host name change > action which eventually calls the > NewServerWizardFragment.checkHostAndServerType() to check for the host name > again. Elson, from your comment I understand that the execution should not enter into checkHostAndServerType() while the timer is running and yet in the method checkHostAndServerType() there is a check: if (manualComp.isTimerRunning() which means that this is not an unexpected behavior. Am I correct? As far as I was able to find out, nothing prevents the following scenario: 1. The timer expires 2. The action code for handling the host name change is being called 3a. NewServerWizardFragment.checkHostAndServerType() is being invoked 3b. At the same moment the host name is being changed again which triggers a new timer Result: The check for timer: ... if (manualComp.isTimerRunning() ... in: NewServerWizardFragment.checkHostAndServerType() returns true and consequently the "Next" button is not enabled. Now, I've gotta figure out how to refactor it to resolve the problem. Created attachment 205399 [details]
move check is timer running in isComplete method
After some time we figured out what is the exact problem:
Timer is run, during execution of the run method in Display.Async is called checkHostAndServerType the timer thread exits and sets the isRunning variable to false. The problem is that checkHostAndServerType also checks isRunning and if it reaches first to the variable before it is set to false the timer is actually canceled and does not execute the validation.
So checkHostAndServerType method is used both in timer and in isComplete methodm, which I think is the root of the problem the isComplete method should return false if the timer is running. So I have devided the usecases so the checkHostAndServerType will do the validation, no matter of the timer state. The timer state will be checked in the isComplete method.
Created attachment 205421 [details]
next patch
wrong turn the isComplete method is used in both cases:(
the behavior is still the same. The timer will trigger async wizard set=Message, which will check the isComplete method. And if this happens before the isRunning flag to be set to false the check will not be completed and thus the next button will remain disabled.
Hi Dimitar, in order to speed up the adoption process, would you mind to list the scenarios that you have tested with the patch "next patch" and does it address the remaining issue causes you to reopen this defect? Hi Elson, I have small SWT bot test that repeatedly tests the next button. It is currently running against our servers, but I can easily adapt it to work against tomcat for example. The scenario in which the problem appears is still the same: 1. Add this extension to server org.eclipse.wst.server.ui.serverCreationWizardPageExtension, where in the immplementation change the host to something different than localhost (yahoo.com for example) 2. Open the server wizard and click between this (enhanced) server and any other that is local Result in some of the cases you will get disabled wizard buttons. best regards, Dimitar Do you mean you have tested it out and the problem no longer exists after the "next patch" that you provided has been applied? The reason that I ask is because I cannot reproduce the problem on my machine so I cannot test out to confirm the fix. Based on the nature of the problem, it may be a timing issue. The code changes looks reasonable to me. If you have confirmed the patch addresses the issue, then I'll adopt the patch as is. Yep the "next patch" solves it. (I had to double checked, just to be sure) the test makes 2000 clicks on the server, without the fix it fails in the first 40 usually, with the fix however it does not fail at all. I am nominating this bug as "hotbug request". We really need it for an SAP adopter product and it drags for too long although we provided patches. Please, consider including this fix in Juno SR2. I agree this should be fixed in 3.4.2. Will drop the code in when smoke test is completed. I checked the history and it turns out I have already dropped the changes to 3.4.2 and 3.5 on Oct.10. I probably forgot to mark the defect as resolved after the drop. Therefore, the fix should be in the current driver already. Sorry for the confusion. Elson, thanks a ton for this! I verified that the fix is included in the latest 3.4.2 M build. New Gerrit change created: https://git.eclipse.org/r/109050 New Gerrit change created: https://git.eclipse.org/r/109135 |