Skip to content
This repository was archived by the owner on Dec 31, 2020. It is now read-only.

Schedule uncommitted reaction cleanup#120

Closed
RoystonS wants to merge 8 commits intomobxjs:masterfrom
RoystonS:schedule-uncommitted-reaction-cleanup
Closed

Schedule uncommitted reaction cleanup#120
RoystonS wants to merge 8 commits intomobxjs:masterfrom
RoystonS:schedule-uncommitted-reaction-cleanup

Conversation

@RoystonS
Copy link
Copy Markdown
Contributor

@RoystonS RoystonS commented Apr 3, 2019

This PR builds on PR #119 and introduces some fixes for #53:

  • a unit test for checking that reactions are properly disposed for components that were double-rendered before being committed (e.g. by ConcurrentMode or StrictMode).
  • a fix which schedules the cleanup of such reactions.

Note that there's one aspect I'm unsure of, both of how to test or fix in a nice way, so I'm pushing this up for feedback. cc: @mweststrate, @FredyC

The happy day case, which I've tested for, and coded for:

  1. render phase 1:
    1. Component instance A gets rendered, creating reaction R1, which we track
    2. we trigger a timer to check for leaked reactions
    3. rendering is aborted and restarts, or StrictMode triggers a second render in order to root out trouble
    4. Component instance A gets rendered, creating reaction R2, which we track
  2. commit phase 1:
    1. Component instance A commits, and we remove R2 from the 'leaked reactions' list
  3. render phase 2:
    1. Component instance B gets rendered, creating reaction R3, which we track
    2. second render triggered
    3. Component instance B gets rendered, creating reaction R4, which we track
  4. commit phase 2:
    1. Component instance B commits, and we remove R4 from the 'leaked reactions' list
  5. cleanup timer ticks:
    1. R1 and R3 are disposed. All is well. Everybody is happy.

But there's an edge case I'm concerned about (which is why I don't think that this PR is sufficient). What if it's been a while since the timer was kicked off, but the latest render phase hasn't had a corresponding commit phase yet?

  1. render phase 1:
    1. Component instance A gets rendered, creating reaction R1, which we track
    2. we trigger a timer to check for leaked reactions
    3. rendering is aborted and restarts, or StrictMode triggers a second render in order to root out trouble
    4. Component instance A gets rendered, creating reaction R2, which we track
  2. commit phase 1:
    1. Component instance A commits, and we remove R2 from the 'leaked reactions' list
  3. render phase 2:
    1. Component instance B gets rendered, creating reaction R3, which we track
    2. second render triggered
    3. Component instance B gets rendered, creating reaction R4, which we track
  4. cleanup timer ticks (between render and commit - this can happen, right?) :
    1. R1, R3 and R4 are disposed
  5. commit phase 2:
    1. Component instance A commits, and the world ends because R4 has previously been disposed by the cleanup schedule

I think, to do this properly, we need to record that a reaction is safe to clean up only once a commit phase has happened?

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.6%) to 98.742% when pulling 6e797ed on RoystonS:schedule-uncommitted-reaction-cleanup into 7d789f1 on mobxjs:master.

@RoystonS
Copy link
Copy Markdown
Contributor Author

RoystonS commented Apr 3, 2019

(Coverage drop is due to removal of last use of onUnmount, as mentioned in #119. Can remove if desired.)

@danielkcz
Copy link
Copy Markdown
Collaborator

danielkcz commented Apr 3, 2019

Wow, this is going to take some time to review for sure :) Also I am slightly confused. Should we merge #119 first? Why two separate PRs? Nevermind, read comment there just now :)

@RoystonS
Copy link
Copy Markdown
Contributor Author

RoystonS commented Apr 3, 2019

Yes, I think #119 could be merged separately, and there may well be value in doing that because, whilst it doesn't solve the leak, it does quieten down StrictMode by avoiding the unnecessary state mutations, making it more useable.

I think the reaction cleanup will take a little longer to work through, and that doesn't need to hold up #119.

If you do merge #119, I'll rebase this PR to tidy it up.

I'm still unhappy about the edge cases, but am still trying to find a way to avoid a multi-render useEffect-based reaction subscribe (i.e. where we only subscribe at commit time.) :-(

@danielkcz
Copy link
Copy Markdown
Collaborator

Can you please resolve conflicts to see the relevant changes only?

@RoystonS
Copy link
Copy Markdown
Contributor Author

RoystonS commented Apr 3, 2019

I've replaced this with PR #121 as I had some issues with my repo and then couldn't push to PR #120.

@RoystonS RoystonS closed this Apr 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants