Bug #21838
closedProject picker search resets
Description
Tasks:
Diagnose/fix issue with search results in the project picker being reset.
Workflow picker "search" resets search when user clicks on an itemmoved to #22153- Project picker contents sometimes resets unexpectedly (possibly because search results are returned out of order)
Old description:
Hello,
when selecting a directory or a file in the run-process view, searching for a project always reset after few seconds, not displaying anymore the filtered ones but all of them.
As additional note, it would be nice to start from the current project rather than from the global view and being able to provide the project or collection uid.
Updated by Peter Amstutz 6 months ago
- 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)
Updated by Peter Amstutz 4 months ago
- Target version set to Development 2024-08-28 sprint
Updated by Peter Amstutz 4 months ago
- Related to Feature #22044: Picker should auto-expand down to the "current" project added
Updated by Peter Amstutz 4 months ago
- Related to Feature #22045: Project/collection/file picker search shows parent projects added
Updated by Peter Amstutz 4 months ago
- Subject changed from Search in workbench/run-process to Selection causing project search to reset
Updated by Peter Amstutz 4 months ago
- Related to Feature #22046: Picker understands UUID and PDH search added
Updated by Peter Amstutz 4 months ago
- Subject changed from Selection causing project search to reset to Picker selection causing project search to reset
Updated by Peter Amstutz 4 months ago
- Target version changed from Development 2024-08-28 sprint to Development 2024-09-11 sprint
Updated by Peter Amstutz 3 months ago
- Target version changed from Development 2024-09-11 sprint to Development 2024-09-25 sprint
Updated by Peter Amstutz 3 months ago
- Target version changed from Development 2024-09-25 sprint to Development 2024-10-09 sprint
Updated by Peter Amstutz 3 months ago
- Subject changed from Picker selection causing project search to reset to Project picker search resets
Updated by Peter Amstutz 3 months ago
- Related to Bug #22153: Workflow picker resets on item selection added
Updated by Peter Amstutz 2 months ago
- Target version changed from Development 2024-10-09 sprint to Development 2024-10-23 sprint
Updated by Stephen Smith 2 months ago
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
Updated by Peter Amstutz 2 months ago
Let me see if I understand the flow.
- Sagas are a new middleware. They are triggered by redux actions.
- 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"
- 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. async
functions (not generators) must be wrapped with a "call" method instead of usingawait
but likeawait
, on success return the value from the resolved promise.- 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)
Updated by Peter Amstutz 2 months ago
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.
Updated by Stephen Smith 2 months ago
- Status changed from In Progress to Resolved
Merged in arvados|c44e03b17a91d85bc219c68cbc625f6a88da58dc
A couple clarifications:- As I understand it,
put
does yield back to the saga scheduler but it's non-blocking so the scheduler I guess normally resumes immediately, so it's only useful for insignificant stuff like loading indicators, not actual work. If you did important work that should be race-free using put, the main saga may finish before the results of the put completes, and when the main saga is called again even if it uses takeLatest, it will be unable to cancel the put since the first saga run already completed. - Call is basically just a way to wrap any function call for the saga scheduler so it can be treated as a unit of work instead of that work being done while the scheduler is waiting for the saga to yield a task (the way call and put work is that they don't actually run anything but just return the task specification object), it's also blocking when the scheduler runs the call task so it guarantees cancellations trickle down which was the important bit here.
Updated by Peter Amstutz 2 months ago
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 likecall
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 likefork
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.