Cannot perform deleteDatabase reliably

[Typhlosaurus]

deleteDatabase works differently on the three supported platforms:

WP8: not supported but reports success.

Android: supported and works if the database is closed. if the database is open it closes the database immediately and removes the runner from the map before trying to delete the database; it does not tell the thread responsible for the database that the database has been closed.

iOS (by inspection, not tested): does not appear to try to close the database but simply tries to delete the file. My guess is that this will fail if the database is open. It also appears not to delete any secondary WAL files, but my knowledge of the iOS API is pretty well non-existent.

These matter as it is currently impossible to determine when a database.close() call has completed (see #109) making it tricky to either close before deleting or close and then perform file delete(s).

[nolanlawson]

This seems like something we should be able to write a unit test for in order to reproduce. We have a unit test that just opens a database and destroys it (which passes on Android and iOS but is ignored on WP8 for now). A unit test that opens, closes, and destroys a database would be a welcome addition.

[Typhlosaurus]

OK

[brody4hire]

Guys, I am planning to submit this plugin to the PhoneGap build early next
week, and what I am thinking is to submit it with db.close() &
deleteDatabase() disabled, until we get these fixed and working properly. I
don't want to delay this any more. These are not part of the W3 draft SQL
API, anyway.

Chris

[nolanlawson]

@brodybits +1. Wouldn't bother disabling, though; how about we just say they're experimental in the README?

[Typhlosaurus]

Fine by me. I probably won't even get a chance to right the unit tests
today.

On 8 August 2014 17:38, Nolan Lawson [email protected] wrote:

@brodybits https://github.com/brodybits +1. Wouldn't bother disabling,
though; just say they're experimental in the README.

—
Reply to this email directly or view it on GitHub
#112 (comment)
.

[Typhlosaurus]

I have a fork with the unit tests for the delete and callback bugs I've reported. I've started working on fixes for android + wp8 (iOS will have to wait).

Please advise: would you prefer that I paste the unit tests here, or provide a pull request or carry on until I can provide the fixes?

[brody4hire]

I have a fork with the unit tests for the various delete and callback bugs
I've found, and am working on fixes for android + wp8 (iOS will have to
wait).

+10

I suspect the callback bug should be a pretty simple fix in the
CoffeeScript.

Please advice: would you prefer that I paste the unit tests here, or
provide a pull request or carry on until I can provide the fixes?

It would be great if you can post the unit tests first. Whether you paste
them or issue a pull request I will make sure you get the credit in the
commit message.

If you issue a pull request on the tests, it is easiest for me if you can
issue the pull request on the common-src branch. Otherwise I will rebase
the changes (which should not be a big deal).

If you issue a pull request on the fixes: for Android and/or iOS, it is
easiest for me if you issue the pull request on the common-src branch
otherwise I would just rebase the changes.

For WP(8), it is easiest for me if you issue the pull request on the
master-src branch.

Many thanks for your help,
Chris

[Typhlosaurus]

Thanks for the advice - I'm new to github and just feeling my way.

I believe that the only one which is a simple js/coffeescript bug is the double invoked executeSql callback. The un-invoked close and open callbacks are also fairly simple, but need to be added to the java (and probably c#) as well as, in the close case, the coffeescript. The android delete issues are java based and require a bit of thought.

[nolanlawson]

@Typhlosaurus Feel free to break it up into many smaller PRs if you are able to. It makes it easier for us to digest when we review your changes.

[nolanlawson]

Also thank you a ton for your work!

[brody4hire]

Thanks for the advice - I'm new to github and just feeling my way.

Yeah I've made some mistakes myself, you can see them if you look through
the history:)

I believe that the only one which is a simple js/coffeescript bug is the
double invoked executeSql callback.

Agreed (I already had a fix in mind). I would kinda like to commit a fix
for this one (#111) today, if possible.

The un-invoked close and open callbacks are also fairly simple, but need
to be added to the java (and probably c#) as well as, in the close case,
the coffeescript.

Yes (#109 & #110). I don't really want to fix these before the first drop
to PhoneGap build.

The android delete issues are java based and require a bit of thought.

Yes (#112) and I don't want to fix these either before the first drop to
PhoneGap build.

@Typhlosaurus https://github.com/Typhlosaurus Feel free to break it up
into many smaller PRs if you are able to. It makes it easier for us to
digest when we review your changes.

I think he already has broken it into a few PRs, we can break them up some
more if we want (+1).

[Typhlosaurus]

Don't thank me for my work until you've seen some! I've just submitted a PR for the unit tests - unfortunately I didn't see the split request until too late. I'll send the subsequent PRs seperately.

[brody4hire]

I'll send the subsequent PRs seperately.

Not necessary as far as I am concerned for the unit tests. I will look at
them and will just include them as they are, if they look ok.

Chirs

[nolanlawson]

@brodybits Goes without saying, but please test on multiple devices before merging, since for the time being we don't have any automated tests. If you need help, I have tons of Android devices myself that I can test on.

Speaking of which, do you think maybe we could get an Open Sauce account? It's awesome when you can just glance at a PR and see whether it had a green check in Travis or not.

[brody4hire]

Absolutely and I would appreciate your help with the Android devices.

I will work on the Open Sauce account, but do not want to hold up the PhoneGap integration for this.

Chris

[brody4hire]

All tests for #109/#110/#112 are passing on the Android version. All tests for this issue (#112) are OK on the iOS version. One failure on the iOS version related to #109/#110.

[brody4hire]

Now fixed inmaster (version 1.0.1)