[WIP] Wait for task no longer

[kbrock]

Overview

Depends upon

• Use params[:name] over session[:async][:params][:name] #9358

Issue

The wait_for_task mechanism relies upon the session.
Every http request changes the session. So when multiple requests come in, updates to the session can overwrite the changes done by another request.

Example:

Host's display of Cap&U was breaking because hits to notifications were overwriting the session variables. Note: VM's display of Cap&U was working fine because it did not have a hit to notifications (and therefore didn't have this race condition).

Solution

Move wait_for_task parameters from the session to the url.

Before

• initiate_wait_for_task sets session[:async][:params] with action and params{}
• browser_refresh_task puts shim into web page
• browser polls wait_for_task waiting for task to complete
• when task is complete, session[:async][:params] is used to complete action

After

• initiate_wait_for_task gathers async_params
• browser_refresh_task puts shim into web page with async_params
• browser polls wait_for_task waiting for task to complete.
• when task is complete, url parameters (async_params) is used to complete action

[kbrock]

@Fryguy This still stores the parameters in the session to be backwards compatible. Do you want to change 10 other controllers and remove this completely from the session?

Pro: eradicates race condition and completely removes session values
Con: This is an extra to 10 controllers (25 lines)

[Fryguy]

I think you need the ||= {} up above to ensure this path exists.

[kbrock]

good eye.
My question below is if we can drop session[:async] all together.
In the meantime, I'll add it back.

[Fryguy]

I don't think I like the async_params coming in as a regular param - not sure why that is needed.

[kbrock]

that is the whole solution.
Otherwise it is a last one in. in our case, it gets blown away by notifications url hit.

[kbrock]

moved over to Task

[Fryguy]

Oh because of this ... hmmmm

[kbrock]

async_parameters have been removed. now it only contains async_interval

[kbrock]

ok, so we have a security problem just passing the action. The send(param[:action]) is pretty wide open. Even if we change to a public_send.

---

The action typically comes from params[:action] which is the url but those are protected.
In other cases, the action is from the url, which keeps getting hit until the task_id is completed.

This new way passes action as a param (user editible) and that is not protected like it is when it comes in via the url.

I can probably change these urls to have a single url with a blocker on a completed task_id (vs having a #{action}_finished pattern.

Aside: Also, these all use rjs. Would have been nice to drop that as well, but this is starting to get serious scope creep.

[kbrock]

WIP: moving parameters into the Task.

It is unfortunate that the task is created elsewhere - so we need to find and then update it

[kbrock]

update:

• moved async_params to MiqTask

WIP: This PR is complete. First commit is from #9358 - after merging, can up-WIP

[kbrock]

update:

• fixed stub in spec to properly return a task_id