obsidian-dice-roller icon indicating copy to clipboard operation
obsidian-dice-roller copied to clipboard

[Bug] TableRoller - cache issue

Open JYCasalis opened this issue 2 years ago • 1 comments

To reproduce the issue do the following:

Step 1: Create a note Note with a table:

| values | 
| ----- | 
|  one  |
|  two |
|  three |
| four |
^table

Add a simple table roller:

`dice: [[Note#^table]]`

Step 2: Open Note a second time (ctrl-click) so that you have the first one in Edit mode, the second one in View mode.

Step 3: Make a small change in the Note by adding a space before or after one of the values (or replace one by twenty if you prefer). It's important that your change does impact the nmuber of characters in the table (so do not just replace one by two :-p).

Step 4: Cross your fingers and re-roll on the Note in view mode. Depending on your "luck", it can take several tries before facing an issue (and that issue might be different from one try to another). Issue can be that the roller found one extra empty line (so once you roll it... ), or considered that the header separator was an entry (----), or many other strange cases. As the roller uses the position of the table block to retrieve it and then to parse it; when the position and length of this block is incorrect, the consequences from a parsing perspective can be.. funny).

Analysis

The problem is linked to the obsidian caching mechanism that, as far as I understood, is done in two steps whether the current implementation was apparently expecting one step only (if I can say so).

The table roller registers a watcher on the Note file to receive modify events. So, each time the Note.md file is changed, the table roller is notified and refreshes its internal list of options available to roll. It does so by ... reading the obsidian cached data. And... you guessed it... despite having received the Event stating that the file has been changed, the cache... has not been changed yet. So the update done by the roller is based on the previous cached version and this can lead to strange issues since this isn't the correct content anymore.

Hum, I said two steps right?

Yes, there is also a way of being notified that the cached version has been changed (this time it is called changed, not modify).

I had not been able to find documentation regarding this caching mechanism so I'm not 100% sure but... I tried replacing the watcher for Note.md / modify: this.plugin.app.vault.on("modify", async (file) by a watcher for the cached file / changed: this.plugin.app.metadataCache.on("changed", async (file)

With this change, the above scenario does not lead to any issue anymore (as far as I've noticed).

Jeremy, does it make sense? (above explanations are not facts, more my "feelings").

JYCasalis avatar May 25 '22 17:05 JYCasalis

OK, so... this is probably worse than I thought 😜

Right now, we add a watcher each time we create a TableRoller. But those are never destroyed! So.. if you open/close the same note 20 times, at the end you'll receive the modify event... twenty times 😁

As far as I understood, the registerEvent ensures that your watchers are removed when the plugin is unloaded. So those watchers should probably be centralized at the plugin level and component should rely on the plugin to be notified (instead of creating watchers themselves).

Still don't know if all of this makes sense 🤔

JYCasalis avatar May 25 '22 18:05 JYCasalis

Yes, this was a major memory leak, can't believe I never noticed it.

Wasn't able to come to a clean workaround, especially since a TableRoller generates additional table rollers in the background, causing a cascade of file watchers. I just took it out and will try to think of a new solution.

valentine195 avatar Oct 26 '22 01:10 valentine195