Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix detecting changes in subfolders for Dropbox #1161

Merged
merged 7 commits into from Mar 2, 2019

Conversation

3 participants
@skddc
Copy link
Member

commented Feb 12, 2019

Fixes #1154
Continues #1158

@iLiviu's original PR description:

I modified the revision cache for files that Dropbox was using until now, and made it generate folder revisions by hashing the revisions of the files in it. The change of revision propagates to parent folders, and forces their revision to update so changes in subfolders can now be detected.

iLiviu and others added some commits Feb 4, 2019

add folder revision generator from cached revision of files
A cache for file revisions, that automatically generates a revision for folders, based on their contents. Useful for backends that do not offer revisions for folders.
fix detecting changes in subfolders for Dropbox
If the root folder had subfolders, and the content of a subfolder changed remotely, the change was not picked up by the Dropbox backend.  This happened because Dropbox does not offer revision ids for folders. 
We now use RevisionCache which automatically generates revisions for folders from locally cached file revisions

@ghost ghost assigned skddc Feb 12, 2019

@ghost ghost added the in progress label Feb 12, 2019

@skddc skddc removed their assignment Feb 12, 2019

@galfert
Copy link
Member

left a comment

Code looks all good to me. Only left one question and a small suggestion.

I haven't tested it myself, as @skddc already confirmed it to work, just reviewed the code itself.

Great work!

Show resolved Hide resolved src/dropbox.js Outdated
Show resolved Hide resolved src/dropbox.js Outdated

@skddc skddc referenced this pull request Feb 14, 2019

Merged

Refactoring sync.js #1147

Improve promise usage
Co-Authored-By: skddc <sebastian@kip.pe>

@ghost ghost assigned skddc Feb 14, 2019

@skddc skddc removed their assignment Feb 14, 2019

fix sync related issues
revert code so when an error occurs in `fetchDelta`, emit the error as a `SyncError`. In addition, notify that the sync is done, so it can be rescheduled.

Also when hooking `sync` function, and `sync` is not yet initialized, hook `syncCycle`, which is called after sync initialization, instead of waiting for `connect` event because in doing so, a sync cycle could have already started and original `sync` is called without being hooked.

@ghost ghost assigned iLiviu Feb 14, 2019

@iLiviu

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

I reverted the code that throws the SyncError, and fixed some other sync issues, however there is still one remaining issue. Right now, if caching is enabled for a path, when sync is initialized it starts the tasks to cache the path automatically, without waiting for a sync cycle. This throws a SyncError since Dropbox's fetchDelta didn't fully complete so local revision cache is not yet available. My question is: should the task to sync the path start immediately after the caching is activated, or should it just add the task and wait for sync to actually start it? What if the user disabled sync, should the remote files still automatically be cached?

This is the referenced code:

this.caching.onActivate(function (path) {
this.addTask(path);
this.doTasks();
}.bind(this));

@skddc

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

This throws a SyncError since Dropbox's fetchDelta didn't fully complete so local revision cache is not yet available

I don't fully understand. Does that mean the local revision cache depends on an HTTP request? It should only depend on local operations, so that the client works when being started offline.

In general, we use the feature initialization in src/remotestorage.js to determine if all parts of the library are ready for action. See this one for example: https://github.com/remotestorage/remotestorage.js/blob/master/src/indexeddb.js#L369-L384

I'm not sure if it makes sense to add the revisioncache to that mechanism, or if another solution makes more sense.

The sync starts with the connected event: https://github.com/remotestorage/remotestorage.js/blob/master/src/sync.js#L1034

And doTasks will actually not do anything if the account isn't connected at the time it's called: https://github.com/remotestorage/remotestorage.js/blob/master/src/sync.js#L935-L947

Otherwise, i.e. for example when an app adds dynamic caching via user input, it would start immediately then.

@skddc

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

Oh btw, the refactored sync.js has been merged to master, so we could merge that back to this branch in order to fix that context bug.

@skddc skddc closed this Feb 15, 2019

@ghost ghost removed the in progress label Feb 15, 2019

@skddc skddc reopened this Feb 15, 2019

@ghost ghost assigned skddc Feb 15, 2019

@ghost ghost added the in progress label Feb 15, 2019

@iLiviu

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

I don't fully understand. Does that mean the local revision cache depends on an HTTP request? It should only depend on local operations, so that the client works when being started offline.

The revision cache is only required when fetching data from remote backend (so when the app is online). It is initialized after Sync is initialized (because it requires hooking the sync function), however the problen is that in the Sync constuctor, there is code that automatically runs the tasks if cache is active. If the tasks were only added, and let the first sync cycle run them (the first sync sycle runs right after the initialization) then this problem would go away, since the sync will be hooked. That is why i asked if it would be normal for library to start caching from remote backend even if user has requested periodic sync to be stopped from the start or it should just add the tasks and if sync is enabled it will run them on next cycle.

@skddc

This comment has been minimized.

Copy link
Member Author

commented Feb 19, 2019

if it would be normal for library to start caching from remote backend even if user has requested periodic sync to be stopped from the start or it should just add the tasks and if sync is enabled it will run them on next cycle

The way you describe it makes this sound like unwanted behavior to me. If sync is turned off, I'd not expect it to run, even if caching.enable() is called. So I would guess we can change that, as long as it doesn't impact normal initial sync in any way.

iLiviu added some commits Mar 1, 2019

do not rely on sync cycle to build local revision cache for Dropbox
When calling `Dropbox#get` with `ifNoneMatch` option and a sync cycle was not completed yet, call `fetchDelta` to build local revision cache instead of throwing an error.
@iLiviu

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

i modified the code so building the revision cache doesn't rely solely on sync cycle anymore, so this fixes throwing SyncError on initialization.
I also merged the latest master for testing.

@skddc

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2019

Cool. LGTM and still works in my app.

thumbs-up-machine

@skddc skddc merged commit 1408142 into master Mar 2, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@skddc skddc deleted the bugfix/1154-dropbox_detect_remote_subfolder_changes branch Mar 2, 2019

@ghost ghost removed the in progress label Mar 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.