[WIP] mail: debug chatter race-condition issue#5166
Draft
alexkuhn wants to merge 321 commits into
Draft
Conversation
|
This PR targets the un-managed branch odoo-dev/odoo:master-owl3-migration, it needs to be retargeted before it can be merged. |
Before this commit, typing composer suggestion like `@` didn't work most of the time. This happens because the suggestion hook had a internal `useState()` that has an internal counter to force re-render the navigable list. However, this counter was not observed, thus it didn't render it. This commit fixes the issue by observing this internal counter in navigable list props, which is used by suggestion. This commit also fixes these related issues in composer suggestions: 1. Suggestion was prompt even with double whitespace. This happens because internal sync of `composerHtml` and `composerText` were trimming the whitespace and thus ignoring the double whitespace. Note that the internal trimming is meant as pre-processing before posting a message but for other cases usually we don't want to trim. 2. Navigable list of suggestion was sometimes not opening automatically, and first item was not automatically pre-selected. This happens because `NavigableList` relies on `useLayoutEffect` with dependency on `props` object, which had its shape change in owl2 but in owl3 that's no longer the case, therefore the `useLayoutEffect` did not work. This is fixed with triggering the open of `NavigableList` on change of `props.options`.
Previous implementation was far too naive: the immediateEffect was triggering the onUpdate even when any field that is read in the function were changed. Also with the update queue, this could register more than a single onChange, which lead to burst of onUpdate that are very prone to bugs from race condition. This commit makes an implementation that follows the logic around the update queue: - a single onChange is registered at once - it untracks the function invoked by onUpdate - when onUpdate is put in queue, the dequeue of onUpdate automatically stops previous onUpdate and registers another one
Improve implementation of compute fields to take the update cycle into account: - properly untrack parts of `immediateEffect` that are outside of the compute method - have reactivity of the `immediateEffect` trigger the `requestCompute`, to comply with update cycle - Ensure single `immediateEffect` thanks to `stopFn` patter like with `onUpdate`'s internal `onChange`. `computeProxy2` has been removed because this is dead code: this proxy was used to scope local reactivity of compute method, but this doesn't work anymore with owl3 and was always the record proxy, which this commit makes it explicit with the removal.
Similar implementation details to `compute` on field.
Before this commit, computed and sorted fields could have their "in-need" flag dropped while the fields are still being "in-need". This happens because there's internal `onChange` to reset the "in-need" flag when the field has changed from computing / sorting, and "in-need" flag is expected to be set again in case the observers still need it. This reset of "in-need" flag makes sense, but there's a scenario where this is not desirable: when the field is computed or sorted on the fly by an "in-need". When this happens, the flag "in-need" is set, but immediately after the `onChange` of computing / sorting is triggering and reset the flag, which incorrectly flag the field as not "in-need". This leads to reactivity problems. This commit fixes the issue by forcing the "in-need" flag at the end of computing /sorting when the action is triggered from "in-need". This preserves the automatic reset of "in-need" flag from change of value computing / sorting while ensuring the "in-need" flag stays when there are still ongoing observers. Note that this "worked" before with owl2 because the observers were more aggressively re-rendering, as shown with tests that show initial render done twice instead or once, or lazy field still continuing computing many steps later even though the observer has stopped observing. This commit fixes the issue. Note that this commit adds an extra "LAZY", i.e. when an observer doesn't register again the lazy field is still computed again at least once. While this looks inefficient, this makes sense with the strategy being used to detect "in-need" observers, which can be immediate when compute is triggered from the "on-need", but if this comes from "in-need" as the flag has to be kept, the "in-need" flag is kept until next compute / sort that updates the value accordingly.
Tests were badly written, using old `reactive()` syntax or were showing wrong behavior from old JS models, fixed by recent commits.
Before this, record.exists() was not reactive. This happens because with owl3 the `record._proxy` is the same for all `reactive()`, which implied that the proxy was always downgraded to `_proxyInternal`. This is a problem because whenever implementation details of JS models were accessing some technical fields, this was made without reactivity. This commit removes downgrade completely, as this no longer works in terms of correctness. The downgrade is motivated for performance reasons, this is probably less costly for owl3 so the removal should be ok. We'll see later if we need some other somewhat similar techniques in case performance is a concern.
- datetimepicker était cassé dans les interactions ; - on pourra avoir 3 builds vert ; - 🤡.
… part' Test was adapted several time. Initially the patch of Message had: - onMounted - onPatched This was then changed to - onMounted - onRendered And this change over-estimated the rendering by at least 1, as onRendered also includes an extra render. But when this change was made, the counter was not increased in the assertion. Then the onMounted was removed, and the counter was reduced. The test is failing because the counter was changed from lower than 3 to lower than 2, but the initial change from onMounted/onPatched to onRendered should preserve the same counter of lower than 3. This commit fixes with the correct counter. Also the assertion on renderers on ActionSwiper were not necessarily made clear that this is related to the Message, so this commit adds comment to make it clear.
Partial revert of [1] Tests were wrongly adapted: the mentions have an extra space when adding the mentions in composer while composing, but the recovering part from starting edition of a message with already some mentions should not have the extra spaces (message content is trimmed). The tests were likely adapted because the tests were not passing due to prior steps that insert mention by hand in composer, and there was probably a issue in owl 3 code where the composer's input value of thread was reused with the extra whitespace. [1]: ecbf491
Uses the many fixes we implemented in tools_js_expression to prevent the parametric t-call script from adding irrelevant template changes. WHY: to save time on future rebases and future client runs of this script
Before this commit, running the whole `@mail` HOOT tests suite leads to heavy memory leak. This happens for one main reason: internal effects in `@mail` are not properly disposed in-between tests. This includes `onChange` utility function, which internally uses `immediateEffect`. 1. `onChange` was returning a proxy, but this return value was never used. This commit now makes it return a function to dispose effects that are internally used. Some components that were using `onChange` now can properly dispose these effects on destroy 2. `onChange` and `Record.onChange` (= a version of `onChange` that follows the store's update cycle) made in store, services, and JS models had no dispose strategy. This commit now adds several functions for `register(Record)OnChange` and `runDisposeFn(s)`, so that all these dispose functions are executed in-between each test. Browser tab RAM of running whole `@mail test`: - before this commit: 2GB - after this commit: 250MB
Continues on the previous html_editor fix, that fixed behavior on mobile but broke on desktop. In the previous fix, we moved the logic to blur on the click event handler but that caused issues on the disktop test because we don't actually click on the iframe input in the test. I tried multiple solutions but many test really on this behavior, so I ended up reverting the useLayoutEffect to how it was before and just adding the bottomsheet rule in the iframe input click which only happens on mobile.
The concerned test was randomly failing before this commit because it didn't properly enforce the flow it tried to reproduce, which is - open a form view with a pager - click "Log a note" to open the composer - click pager next to go to the next record - on that record, type "@" in the composer to mention people However, before this commit, we typed "@" in the composer without waiting to be on the second record. So it might happen that we type "@" while still being on the first record, and then tried to do assertions on the mention suggestion dropdown while being on the second record (which thus contains an empty composer).
…server In Owl3, effect() runs its callback immediately and synchronously on creation to establish reactive tracking. This means willUpdateRecord fires during setup() itself, before this.lastAccount and this.lastProduct were initialized, so both were undefined. shallowEqual(undefined, value) returns false, making accountChanged and productChanged always true on the initial run, triggering a spurious fetchAllPlans RPC for every newly constructed AnalyticDistribution component. In Owl2, the master useRecordObserver used effect(fn, [deps]) + batched + RAF debouncing, deferring the first callback until after setup() completed, so lastAccount/lastProduct were already set. Fix: move lastAccount/lastProduct initialization before the useRecordObserver call.
mcm-odoo
force-pushed
the
master-owl3-migration
branch
from
f05caf0 to
be0db09
Compare
taking advantage of owl's odoo/owl@d66ce3c useLayoutEffect in Odoo is adapted to not subscribe to those signals
1. Run test with dev tools until last console is RPC store data 2. `window.aku2 = window.aku.__owl__` to track fiber assign of Chatter => observe cancel of fiber from FromRenderer
alexkuhn
force-pushed
the
master-owl3-migration-chatter-aku
branch
from
681d442 to
f802f75
Compare
ged-odoo
force-pushed
the
master-owl3-migration
branch
2 times, most recently
from
c7f9086 to
58e9fc7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
11 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
window.aku2 = window.aku.__owl__to track fiber assign of Chatter=> observe cancel of fiber from FromRenderer