Skip to content

Fixing sychronization and concurrency issues#123

Merged
stefan-niedermann merged 28 commits into
nextcloud:masterfrom
korelstar:sync-bugfix-refactoring
Aug 13, 2016
Merged

Fixing sychronization and concurrency issues#123
stefan-niedermann merged 28 commits into
nextcloud:masterfrom
korelstar:sync-bugfix-refactoring

Conversation

@korelstar

@korelstar korelstar commented Jul 10, 2016

Copy link
Copy Markdown
Member

This is the current state of my attempt to fix #117. It's not fully finished yet, but the majority of the work is done. Therefore, please have a look at this pull request and test it with all the test cases you can imagine. I will update the pull request continually.

Finished Tasks

  • Database: new column REMOTE_ID (fixes conflicts between parallel creation of different notes locally and remotely)
  • DBStatus: LOCAL_CREATED is not used anymore (distinction if Note is new or edited can be made with checking REMOTE_ID=0; fixes conflicts on synchronization and parallel local edits)
  • Note: model is split in to DBNote (representing the local DB entry of a note) and OwnCloudNote (representing the remote note, used by NotesClient).
  • ItemAdapter/note item layout: show icon for unsynchronized changes (simplifies debugging, includes all changes done in Show icon if a note is not synchronized (hint to a possible error) #121, closes Show an error message if synchronization fails #119)
  • NoteSQLiteOpenHelper: database upgrade (add the new column and adapts the data without flushing the database)
  • NoteSQLiteOpenHelper: add more constraints for database updates (fixes lost updates due to parallel changes of notes (from synchronization and user interface))
  • NoteSQLiteOpenHelper: updateNoteAndSync(): only make database changes, if the content really changed (fixes don't autosave if the note isn't modified #104, at least the original intention of the author).
  • NoteSQLiteOpenHelper: remove recurrent code (improves code quality)
  • NoteServerSyncHelper: only update if network is connected (fixes lots of thrown exceptions)
  • NoteServerSyncHelper: schedule synchronization requests (if synchronization is active or network not connected; fixes missing push of all outstanding changes)
  • NoteServerSyncHelper: restructure the communication between synchronization task and user interface.
  • NoteServerSyncHelper: consolidate synchronization AsyncTasks into a single one (improves code quality)
  • EditNoteActivity: ensures, that auto-save is not called, if the previous auto-save is still running. In addition, start next auto-save not before a small delay has passed. (This could fix Editing saves on every keypress. It hammers my connection. #76)

Other Issues I've Found (not covered by this pull request)

  • There is no conflict resolution for the case that a Note is locally and remotely edited in parallel (e.g. during a longer time without network connection). Currently, local changes overwrite remote changed. We have to discuss what would be the desired behavior. See also Handle remote note deleted #66.
  • The ItemAdapter should use a CursorAdapter which is updated automatically if the backend data is changed.
  • In order to save traffic during synchronization, pullRemoteChanges() could use the exclude parameter of the Notes API in a first step to check which notes changed without downloading unchanged notes. Only in the second step, the whole note content would be downloaded from the changed note. This could save traffic, however, it has to be investigated if the effort worth it (what is the typical/topmost number and size of notes?).

korelstar added 15 commits June 26, 2016 14:25
…String sql, String[] selectionArgs)

New method getLocalModifiedNotes() is a preparation for bugfixing #117
… fix several bugs belonging concurrency and synchronization. Outstanding: documentation, testing, cosmetic changes
…String sql, String[] selectionArgs)

New method getLocalModifiedNotes() is a preparation for bugfixing #117
… fix several bugs belonging concurrency and synchronization. Outstanding: documentation, testing, cosmetic changes
…nterface using callbacks, only try to sync if not offline, otherwise show an error message.
@stefan-niedermann

Copy link
Copy Markdown
Member

I think i have some free time at this weekend. I'll test this, merge this and if all is going well, i'll build a new version for F-Droid.

@stefan-niedermann

Copy link
Copy Markdown
Member

looks good. shall i wait for the three open tasks or do prefer to merge now?

@korelstar

Copy link
Copy Markdown
Member Author

I tested my branch again and I found an embarrassing bug. Unfortunately, I can't fix before Monday. So please wait a few days. I'll write a comment here when I'm finished.

@korelstar

Copy link
Copy Markdown
Member Author

ready to merge :-)

@stefan-niedermann stefan-niedermann merged commit d9b3d87 into nextcloud:master Aug 13, 2016
@moshpirit

Copy link
Copy Markdown

\o/ wooohoo! Thanks a lot for the support!!

@stefan-niedermann

stefan-niedermann commented Aug 30, 2016

Copy link
Copy Markdown
Member

@korelstar just let me say a huge thank you again - i just had time again to watch your code and it looks awesome.
I want to tag a new version for F-Droid since the current version 0.8.1 is buggy. One little thing i noticed: If you are doing a clean install of the app (or purge any data of it) and start it, you will get into the settings activity - that's fine, but it trys to sync and fails (and shows an error message).
I don't have a problem if it trys a sync on first start-up, but it should at least not display an error message.

screenshot_20160830-194528

@korelstar

Copy link
Copy Markdown
Member Author

I will have a look at this issue and fix it as soon as possible, but I can't tell when, since I just changed my residence and I haven't access to my working station yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants