yahoo / squidb

SquiDB is a SQLite database library for Android and iOS
https://github.com/yahoo/squidb/wiki
Apache License 2.0
1.31k stars 132 forks source link

SQLiteConnection object leak #106

Closed zsoltk closed 9 years ago

zsoltk commented 9 years ago

Hello,

I'm trying to investigate why I get SQLiteConnection leak warnings using SquiDB.

At its simplest, I get the warning in a following scenario:

  1. I start a Service from an Activity
  2. I fetch one or more items using SquiDB in the Service

I threw together an absolutely minimal test project here: https://github.com/zsoltk/squidb-sqliteconnection-leak

Steps to recreate:

  1. Run app
  2. Click "Start example service" button
  3. Send the app to background (e.g. press Home) and wait a few seconds for the warning to appear in Logcat

Logcat lines should read:

D/MainActivity: Starting ExampleService
D/ExampleService: Fetching from db
W/SQLiteConnectionPool: A SQLiteConnection object for database '/data/data/com.example.foobar/databases/foo.db' was leaked!  Please fix your application to end transactions in progress properly and to close the database when it is no longer needed.

The last line should be repeated exactly as many times as many times you pressed the button. Running the service four times creates four leaks.

Relevant lines from the test project code:

Activity:

public class MainActivity extends AppCompatActivity {

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_main);

        Button button = (Button) findViewById(R.id.button);
        button.setOnClickListener(new View.OnClickListener() {
            @Override
            public void onClick(View v) {
                startExampleService();
            }
        });
    }

    private void startExampleService() {
        Log.d(getClass().getSimpleName(), "Starting ExampleService");

        Intent intent = new Intent(this, ExampleService.class);
        startService(intent);
    }
}

Service:

public class ExampleService extends IntentService {
    public ExampleService() {
        super("ExampleService");
    }

    @Override
    protected void onHandleIntent(Intent intent) {
        Log.d(getClass().getSimpleName(), "Fetching from db");
        Database database = new Database(this);
        database.fetch(Foo.class, 1L);
    }
}

I tested on a Lollipop physical device, Lollipop emulator, KitKat emulator, Jelly Bean emulator. The warning is there on both Lollipop instances, but does not appear on the others. So either the leak itself relates to Lollipop, or the warning message didn't exist in earlier platforms (lack of knowledge here).

I also don't know whether the issue has anything to do with services at its core, but at least there's no warning in a normal Activity context.

Any idea why this happens? Is there anything I'm doing wrong?

sbosley commented 9 years ago

Thanks for the test project! We'll take a look.

sbosley commented 9 years ago

Ok, it looks to me like the issue here is that your database instance is not an app-wide singleton as recommended by this wiki page. A instance is getting created in the service that is never closed, and when it is collected by the garbage collector that warning is printed by the OS. I can actually make the warning print from your sample app on a KitKat emulator too by adding a System.gc() call after the call to fetch and clicking the button a couple times.

We mention in various places in the documentation that your database instances should be created as singletons, although perhaps we should be more explicit that we mean global singletons -- i.e., one instance per database for the lifetime of the application. You'll have to decide what kind of singleton management makes sense for your app -- we use a dependency injector to manage our singletons, but you could also maintain a static instance in an application subclass, or some other strategy.

jdkoren commented 9 years ago

My findings were basically the same as @sbosley's.

sbosley commented 9 years ago

One more thing to note -- if you keep your database instance as a singleton, you actually shouldn't need to worry about closing it explicitly. See this stack overflow post.

zsoltk commented 9 years ago

Right! Thanks for the info, now it's all clear.