AsteroidOS / AsteroidOSSync

Android application to synchronize a phone with a watch running asteroid-btsyncd.
GNU General Public License v3.0
101 stars 37 forks source link

nested sync()/unsync() calls #206

Closed argosphil closed 1 year ago

argosphil commented 1 year ago

Hi! It's quite possible I'm missing something silly, since this is the first Android development I've done, but I tried writing a new service and noticed that the sync()/unsync() were sometimes nested: We'd see

newService.sync();
newService.sync();
newService.unsync();

I don't know whether that is intentional or not, but I am pretty sure it conflicts with the way the code is written. For example, in ScreenshotService.java, we have:

    @Override
    public void sync() {
        mSReceiver = new ScreenshotReqReceiver();
        IntentFilter filter = new IntentFilter();
        filter.addAction("org.asteroidos.sync.SCREENSHOT_REQUEST_LISTENER");
        mCtx.registerReceiver(mSReceiver, filter);

        mDownloading = false;
    }

    @Override
    public void unsync() {
        try {
            mCtx.unregisterReceiver(mSReceiver);
        } catch (IllegalArgumentException ignored) {}
    }

If sync() is called twice without an intervening unsync(), mSReceiver will be overwritten, and the first mSReceiver to be created will never be unregistered. This accumulates on every device connection/disconnection until AsteroidOSSync becomes unusable, I believe.

AM I missing something? I've got a branch which fixes all the services not to assume sync()/unsync() are never nested, but maybe it would be better to change the generic code never to ensure strict sync()/unsync()/sync()/unsync() call order...

jrtberlin commented 1 year ago

Thank you for bringing this up. I don't think that you are missing something.