search

digidem / mapeo-core

Library for creating custom geo data and syncronizing via a peer to peer network
23 stars 2 forks source link

fix: Close syncfile properly after an invalid sync attempt #132

Closed achou11 closed 2 years ago

achou11 commented 2 years ago

Addresses https://github.com/digidem/mapeo-desktop/issues/612

Preview (when patched on Mapeo Desktop):

612-syncfile-data-loss

gmaclennan commented 2 years ago

Had a read into this and a read through the source code of osm-p2p-syncfile. We need to ensure that we call syncfile.close() within any error that is handled after the syncfile instance is created (https://github.com/digidem/mapeo-core/blob/ac/close-syncfile-after-invalid/sync.js#L475), although I'm not sure the state the syncfile is in after an error from syncfile.ready(). We need to do this because the other errors will also leave the syncfile in a "damaged" state. I really don't like the way this works right now because it is really vulnerable to bugs, but we can patch this up for now and make sure we fix the way we sync to files in the future to make it more robust.

achou11 commented 2 years ago

Had a read into this and a read through the source code of osm-p2p-syncfile. We need to ensure that we call syncfile.close() within any error that is handled after the syncfile instance is created (https://github.com/digidem/mapeo-core/blob/ac/close-syncfile-after-invalid/sync.js#L475), although I'm not sure the state the syncfile is in after an error from syncfile.ready(). We need to do this because the other errors will also leave the syncfile in a "damaged" state. I really don't like the way this works right now because it is really vulnerable to bugs, but we can patch this up for now and make sure we fix the way we sync to files in the future to make it more robust.

Makes sense. Are you suggesting I add those patches to this PR, in addition to what I already added?

gmaclennan commented 2 years ago

Makes sense. Are you suggesting I add those patches to this PR, in addition to what I already added?

Yes. I am proposing that syncfile.close() is called within the error handler onerror. However this will require a minor refactor to create a separate error handler because the current one is outside the scope of syncfile. If not then we will just hit this same bug when a different error happens.

achou11 commented 2 years ago

Yes. I am proposing that syncfile.close() is called within the error handler onerror. However this will require a minor refactor to create a separate error handler because the current one is outside the scope of syncfile. If not then we will just hit this same bug when a different error happens.

Attempted to clean up the implementation a bit so that the the file is closed whenever an error occurs. I ran the formatter on the file so the diff is a little bit hard to parse quickly, so I'd recommend just looking at https://github.com/digidem/mapeo-core/pull/132/commits/5bfd8d7ac926cc330426e40d77171eed53b145cd for the refactoring work

gmaclennan commented 2 years ago

Looks good, just a small bug that needs fixed.

achou11 commented 2 years ago

Optional callback onClose will not be called if err is truthy.

Spoke offline about this and this is desired behavior based on previous implementation, just needed some cleanup to make it clearer (see https://github.com/digidem/mapeo-core/pull/132/commits/8b497c37715aa3339c301c678a772273ccb65c44)