> On Sept. 7, 2016, 4:56 a.m., Alexandr Antonenko wrote:
> > In step-4 the flag "nextBtnClickInProgress" was not set anywhere -> We set 
> > nextBtnClickInProgress in router for step 4
> > 
> > So if a timeout was manually added in the "submit" function of the 
> > controller, we could allow multiple clicks to be registered -> I think this 
> > is not the right way to test and create bugs based on such synthetic test 
> > case. Because if I will wrap with timeout your newly added line where you 
> > set nextBtnClickInProgress to true, results will be the same, I will be 
> > able to perform multiple clicks.
> > 
> > I was not able to reproduce this issue manually on latest trunk. 
> > 
> > This behavior is reproducible only when the host is slow, which would allow 
> > enough time to click the Next button multiple times -> Even when I set 
> > throttling to 50 kb/s and delay 500ms (the slowest setting in chrome 
> > network tab) I'm unable to reproduce
> > 
> > I think what we have in router for step 4 is enough to prevent multiple 
> > clicks

Hello Alexandr,
The "nextBtnClickInProgress" flag is set in the router when we call the 
App.router.send('next') from the controller. This misses out the case when the 
point of execution is still in the controller validating dependencies with a 
callback function. Due to the presence of this callback, there is a possibility 
for context switch and thus, allowing the registered multiple clicks to execute 
the same "submit()" function in the controller.

The addition of the timeout was only to simulate the lag from the host machine 
in performing the validating functions like service dependency checks or some 
other validation functions in "validate()". 

This issue is difficult to reproduce and depends heavily on how fast the host 
machine responds back (It is not related to the browser because the multiple 
clicks, when registered, do succeed in executing the "submit()" function those 
many times and hence skipping multiple steps via the App.router.send('next') 
call.) 

This is my understanding of why and how the issue is still present, but I have 
also not been able to reproduce this behavior on the latest trunk. I had to 
revise the fix for an earlier branch (where I was able to reproduce it only 
once) when I realized that the fix is not perfect.


- Keta


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51677/#review147972
-----------------------------------------------------------


On Sept. 6, 2016, 10:41 p.m., Keta Patel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51677/
> -----------------------------------------------------------
> 
> (Updated Sept. 6, 2016, 10:41 p.m.)
> 
> 
> Review request for Ambari, Alexandr Antonenko and Di Li.
> 
> 
> Bugs: AMBARI-18327
>     https://issues.apache.org/jira/browse/AMBARI-18327
> 
> 
> Repository: ambari
> 
> 
> Description
> -------
> 
> While installing a cluster if the Ambari user happened to click the "Next" 
> button of Step-4 (Choose Services step) more than once, then it could lead to 
> skipping directly to step-6 or 7 or 8 depending on the number of times the 
> click got registered. This behavior is reproducible only when the host is 
> slow, which would allow enough time to click the Next button multiple times.
> 
> This issue is related to AMBARI-14574, and is a revised fix for the Step-4.
> 
> 
> Diffs
> -----
> 
>   ambari-web/app/controllers/wizard/step4_controller.js 04e6784 
> 
> Diff: https://reviews.apache.org/r/51677/diff/
> 
> 
> Testing
> -------
> 
> **FIX:**
> In step-4 the flag "nextBtnClickInProgress" was not set anywhere. So if a 
> timeout was manually added in the "submit" function of the controller, we 
> could allow multiple clicks to be registered and these would be processed 
> allowing steps to be skipped. In the revised fix, the flag 
> "nextBtnClickInProgress" is set in the "submit" function. 
> "nextBtnClickInProgress" causes "isSubmitDisabled" function also to return 
> "true" or "false" depending on whether it is "true" or "false" respectively. 
> For this reason, we also need to reset the "nextBtnClickInProgress" flag at 
> the end of "validate" function so that the subsequent valid "submit" calls, 
> on closing pop-up(s) can be processed. 
> 
> **TESTING:**
> ambari-web unit tests affter applying the patch:
> 
>   30182 tests complete (46 seconds)
>   151 tests pending
> 
> 
> Also, did manual testing by adding a timeout in the submit() of 
> step4_controller.js to allow multiple clicks to be registered on the Next 
> button.
> 
> 
> Thanks,
> 
> Keta Patel
> 
>

Reply via email to