Arvados

• Workflow picker "search" resets search when user clicks on an item (#22153)
• Project picker contents sometimes resets unexpectedly (possibly because search results are returned out of order)
• Want pickers to start from the "current" project (#22044)
• When looking at project search results, want a way to navigate to the owner "above" a project (#22045)

Changes at arvados|c104cd75c791f5a0781b42d77f4b6f0bcecbd5c7 branch 21838-picker-search-race
Tests developer-run-tests-services-workbench2: #1280

• All agreed upon points are implemented / addressed.
  • Added redux saga, and a root saga that automatically restarts any that crash - shouldn't happen but it will spit a console error if it does
  • Converted loadProject into a saga with 2 entry point watchers, loadProject and loadSearch, the latter featuring automatic cancellation of running instances when called
  • Removed setSearch triggering thunk and replaced it with a saga that calls the reducer and then kicks off the race-free search saga
  • Changed all calls to the setSearch reducer to call the saga instead
  • Converted refreshTree into a saga, combined the multiple set collection filter action calls into a single saga that fans out in order to eliminate race conditions with takeLatest
  • Converted loadFavorites and loadPublicFavorites into sagas to complete converting all load methods used in refreshPicker into sagas - this makes it easier to ensure the refresh method blocks and passes along cancellation to the final methods (as I understand it, the only way to dispatch thunks from sagas is to pass in dispatch which seems like an anti-pattern, and the easy way to call it using put() is non-blocking)
• Anything not implemented (discovered or discussed during work) has a follow-up story.
  • none
• Code is tested and passing, both automated and manual, what manual testing was done is described
  • manually tested by adding delays to the service methods and disabling the input field debounce, logging in the finally{if(cancelled())} of the saga used with takeLatest shows cancellations
• integration tests seem to already test the search inputs, and refresh is indirectly tested by the load initial value tests
• Documentation has been updated.
  • none
• Behaves appropriately at the intended scale (describe intended scale).
  • no change in scale
• Considered backwards and forwards compatibility issues between client and server.
  • no relevant changes
• Follows our coding standards and GUI style guidelines.
  • no ui changes

Let me see if I understand the flow.

1. Sagas are a new middleware. They are triggered by redux actions.
2. Sagas have their own stack of methods called "Watchers". Each watcher yields a generator function, these generator functions run in a loop, each time a new action appears, each watcher is iterated in its loop where it tries to "take" the action (if it matches) and may launch a "saga" (another generator function), possibly cancelling a previous "saga"
3. The saga gets the matching action and does stuff, such as put further redux actions. Until nested dispatch() calls, put yields control back to the saga "scheduler" and only resumes when the action is completed.
4. async functions (not generators) must be wrapped with a "call" method instead of using await but like await, on success return the value from the resolved promise.
5. Presumably, anywhere there is a "yield" it returns controller to the scheduler, and if the function is cancelled, it won't ever be resumed.

(looking at call a little, more, it seems like it is also used for sagas to call other sagas)

I think this is good to merge.

I think we will want to convert more of these kinds of middleware to sagas.

This makes me think about the fundamental issue that any time do you have await or .then(), when the method resumes the world may have changed. So the nice thing about sagas is you can have it restart processing in that case and abandon the old thread. As a follow-up task we should audit everywhere we use await or .then() and think about what could change between the call and the callback to invalidate assumptions.

The other thing I was thinking about is that redux seems to prefer "fat" actions. Any time we change the store it potentially does an expensive re-render (at minimum, it has to go through every connected component and mapStateToProps and then compare the new props to the old props). So having a "fat" action that changes multiple things in the store at once reduces that work.

For example, in receiveTreePickerData it dispatches LOAD_TREE_PICKER_NODE_SUCCESS followed by EXPAND_TREE_PICKER_NODE, but there's no reason LOAD_TREE_PICKER_NODE_SUCCESS couldn't have an "expand" parameter so that it can set the node contents and expand it in the same action. (Not suggesting this should be done in this ticket, but I have been thinking a lot about react/redux optimization lately.) Similarly, sagas should probably avoid making changes to the store until the whole thing has completed, so if the saga is abandoned you don't leave half-changed state lingering around. On the other hand, things like setting the "loading" status to show a spinner do need to change the state mid-saga, but if that is cancelled, we don't want to leave it in "loading" state forever.

Writing this down for my understanding / future reference.

https://redux-saga.js.org/docs/api/

put(action)​

Creates an Effect description that instructs the middleware to schedule the dispatching of an action to the store. This dispatch may not be immediate since other tasks might lie ahead in the saga task queue or still be in progress.

You can, however, expect the store to be updated in the current stack frame (i.e. by the next line of code after yield put(action)) unless you have other Redux middlewares with asynchronous flows that delay the propagation of the action.

Downstream errors (e.g. from the reducer) will be bubbled up.

What this seems to be saying is that it isn't a direct call to the store the way dispatch is, but it won't resume the saga until the action has been dispatched.

Their definition of "non-blocking" seems to mean "does not wait for promises to be resolved". There's also a putResolve which does wait for promises.

Regarding call it seems like there's three major control flow methods:

• call takes a method and invokes it and if the method returns a promise or an iterator, it waits for resolution/keeps iterating to completion before resuming the caller. If the caller task is cancelled, that propagates down to any called tasks as well.
• fork is like call except that it is allowed to run concurrently, but is still associated with the caller and will be cancelled if the parent task is cancelled.
• spawn is like fork except that it is independent of the parent task

Overall, while the implementation is kind of tricky and requires some boilerplate, the ability to reason about these kind of ongoing tasks is very useful.