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

Dependency injection / module architecture broke all thread safety #118

Closed
foooorsyth opened this issue Aug 23, 2020 · 6 comments
Closed

Comments

@foooorsyth
Copy link
Contributor

Thread safety across the SDK was lost when synchronized methods from the old Countly god-object were moved into their respective modules. These methods are now synchronized with different locks (their interface, an inner class of the module), and are therefore no longer thread safe. Thread safety can be regained trivially by using the Countly instance passed to each module as the module and interface wide lock.

eg.

public class ModuleViews extends ModuleBase {
    @Inject Countly _cly;

    public class Views {
        public synchronized Countly recordView(String viewName) {
            // method contents...
        }
    }
}

should become

public class ModuleViews extends ModuleBase {
    @Inject Countly _cly;

    public class Views {
        public Countly recordView(String viewName) {
            synchronized(_cly) {
                // method contents...
            }
        }
    }
}
@ArtursKadikis
Copy link
Member

ArtursKadikis commented Aug 23, 2020

Do you have any sample situations where the current approach is causing issues?

@foooorsyth
Copy link
Contributor Author

This SDK has never explicitly aimed to be usable from multiple threads at the same time and ideally would be called from a single thread or even the main thread if ratings would be used

This is a confusing to me. If this is the case, why does almost every method in Countly/ Module* and almost every write method in CountlyStore have the synchronized keyword? And why is synchronization explicitly mentioned in comments in both EventQueue and ConnectionQueue? Definitely seems like thread-safety was intended at some point.

If thread-safety is not intended, it would make sense to remove the synchronized keyword from these method signatures. Alternatively, you could add @MainThread to just have people use main.

I don't have a specific situation in which it was crashing. I haven't been working on the project much in the past few months and just recently noticed that the module setup broke thread safety that used to exist as all of those methods used to be in the same class.

@ArtursKadikis
Copy link
Member

We do try to make it as thread safe as possible.

@ArtursKadikis
Copy link
Member

I guess I had someting else in mind when talking about "explicit multi threading".

I'll look into improving the current threading protections.

@ArtursKadikis
Copy link
Member

Hello. Just a fyi, this commit should fix this issue:
10631de

@ArtursKadikis
Copy link
Member

Fix released

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants