Last Comment Bug 583882 - Need a way to clone an existing connection
: Need a way to clone an existing connection
Status: RESOLVED FIXED
: dev-doc-complete
Product: Toolkit
Classification: Components
Component: Storage (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla2.0b5
Assigned To: Shawn Wilsher :sdwilsh
:
:
Mentors:
Depends on: 519769
Blocks: 599980 582703 599978
  Show dependency treegraph
 
Reported: 2010-08-02 15:23 PDT by Shawn Wilsher :sdwilsh
Modified: 2010-09-27 13:22 PDT (History)
6 users (show)
sdwilsh: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta5+


Attachments
v0.1 (9.40 KB, patch)
2010-08-02 17:16 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review
v1.0 (10.71 KB, patch)
2010-08-03 11:58 PDT, Shawn Wilsher :sdwilsh
bugmail: review+
Details | Diff | Splinter Review
v2.0 (15.72 KB, patch)
2010-08-11 12:57 PDT, Shawn Wilsher :sdwilsh
bugmail: review+
Details | Diff | Splinter Review
v2.1 (17.34 KB, patch)
2010-08-23 15:34 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review
v2.2 (16.99 KB, patch)
2010-08-23 15:47 PDT, Shawn Wilsher :sdwilsh
shaver: superreview+
Details | Diff | Splinter Review
v2.3 (17.72 KB, patch)
2010-08-24 10:41 PDT, Shawn Wilsher :sdwilsh
bugmail: review+
Details | Diff | Splinter Review
v2.3 to v2.4 interdiff (1.65 KB, patch)
2010-08-24 14:37 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review
v2.5 (17.77 KB, patch)
2010-08-25 14:40 PDT, Shawn Wilsher :sdwilsh
no flags Details | Diff | Splinter Review

Description Shawn Wilsher :sdwilsh 2010-08-02 15:23:07 PDT
We should have a way to clone an existing connection so consumers don't have to do all the work again.  Additionally, provide an option for it to be a read only connection to support multiple readers and one writer.
Comment 1 Shawn Wilsher :sdwilsh 2010-08-02 17:16:23 PDT
Created attachment 462266 [details] [diff] [review]
v0.1

This is basically there, but I think I found a bug in SQLite.  If you open a db with the shared cache, and then clone, it doesn't matter if you make it read only, you can still write to it.  Pinged drh about it, but he's not around atm.  This is a wip, but is likely reviewable.
Comment 2 Shawn Wilsher :sdwilsh 2010-08-03 11:32:10 PDT
OK, so this is intentional in SQLite.  So, I'll add some notes to the idl, add some tests so we know if this ever changes, and then carry on.
Comment 3 dwitte@gmail.com 2010-08-03 11:37:54 PDT
Now I'm curious... why?
Comment 4 Shawn Wilsher :sdwilsh 2010-08-03 11:39:29 PDT
(In reply to comment #3)
> Now I'm curious... why?
Because these are just flags that they pass to open.  The cache is what opens the database, and since the first one is readwrite, they all will be.  They'll look into making it cache-level instead of file level.
Comment 5 Shawn Wilsher :sdwilsh 2010-08-03 11:58:09 PDT
Created attachment 462455 [details] [diff] [review]
v1.0
Comment 6 Shawn Wilsher :sdwilsh 2010-08-10 16:31:45 PDT
This patch is missing something important - we should be copying over functions added to the connection too (but busy handlers should not be copied).
Comment 7 Shawn Wilsher :sdwilsh 2010-08-11 12:57:01 PDT
Created attachment 464906 [details] [diff] [review]
v2.0

This ended up being a bit more invasive than I had hoped, but I have full test coverage on the changes, so I'm pretty confidant that it's safe.  This copies over any added functions to the new connection as well.
Comment 8 Andrew Sutherland [:asuth] 2010-08-12 00:46:55 PDT
Comment on attachment 464906 [details] [diff] [review]
v2.0

Might be worth putting a comment in the impl that we will attempt to copy over the functions from registerFunctions() but since they are already bound at the target (registerFunctions is invoked as part of Connection::initialize) their registration call will fail but we don't care which is why we cast the return value of that call to void.

Or not, but it could be surprising to the hypothetical someone who unregisters our default functions to replace them with their own and then discovers that the clone has the original set of functions again and then goes to read the code and is none the wiser.
Comment 9 Shawn Wilsher :sdwilsh 2010-08-12 08:23:58 PDT
(In reply to comment #8)
> Might be worth putting a comment in the impl that we will attempt to copy over
> the functions from registerFunctions() but since they are already bound at the
> target (registerFunctions is invoked as part of Connection::initialize) their
> registration call will fail but we don't care which is why we cast the return
> value of that call to void.
> 
> Or not, but it could be surprising to the hypothetical someone who unregisters
> our default functions to replace them with their own and then discovers that
> the clone has the original set of functions again and then goes to read the
> code and is none the wiser.
That's an interesting case I hadn't thought about.  Will add the comment.
Comment 10 Shawn Wilsher :sdwilsh 2010-08-12 16:41:06 PDT
(In reply to comment #8)
> Might be worth putting a comment in the impl that we will attempt to copy over
> the functions from registerFunctions() but since they are already bound at the
> target (registerFunctions is invoked as part of Connection::initialize) their
> registration call will fail but we don't care which is why we cast the return
> value of that call to void.
> 
> Or not, but it could be surprising to the hypothetical someone who unregisters
> our default functions to replace them with their own and then discovers that
> the clone has the original set of functions again and then goes to read the
> code and is none the wiser.
Alternatively, I could just call removeFunction if Create fails, and then try again (silently failing still).  I think this is actually probably better, agree?
Comment 11 Andrew Sutherland [:asuth] 2010-08-13 07:12:35 PDT
(In reply to comment #10)
> Alternatively, I could just call removeFunction if Create fails, and then try
> again (silently failing still).  I think this is actually probably better,
> agree?

Yes.
Comment 12 Shawn Wilsher :sdwilsh 2010-08-23 15:14:14 PDT
Bug 582703 is now blocking, so this does too.
Comment 13 Shawn Wilsher :sdwilsh 2010-08-23 15:34:38 PDT
Created attachment 468499 [details] [diff] [review]
v2.1

per comment 10, with a test that overrides lower.  asuth, feel free to take a look at this, but I don't think there is much value in you doing so, so I'm not going to request another review.
Comment 14 Shawn Wilsher :sdwilsh 2010-08-23 15:46:09 PDT
(In reply to comment #8)
> Comment on attachment 464906 [details] [diff] [review]
> v2.0
> 
> Might be worth putting a comment in the impl that we will attempt to copy over
> the functions from registerFunctions() but since they are already bound at the
> target (registerFunctions is invoked as part of Connection::initialize) their
> registration call will fail but we don't care which is why we cast the return
> value of that call to void.
> 
> Or not, but it could be surprising to the hypothetical someone who unregisters
> our default functions to replace them with their own and then discovers that
> the clone has the original set of functions again and then goes to read the
> code and is none the wiser.
Actually, it won't fail.  sqlite lets you redfine this way, and my test confirms this.  Taking that code I added back out, and leaving the test in.
Comment 15 Shawn Wilsher :sdwilsh 2010-08-23 15:47:58 PDT
Created attachment 468502 [details] [diff] [review]
v2.2

per comment 14
Comment 16 Mike Shaver (:shaver -- probably not reading bugmail closely) 2010-08-23 16:14:12 PDT
Comment on attachment 468502 [details] [diff] [review]
v2.2

sr=shaver
Comment 17 Shawn Wilsher :sdwilsh 2010-08-23 22:23:14 PDT
Things I did not expect to happen:
Running the next test: test_clone_readonly
Assertion failed: (isCreate==0 || isReadWrite), function unixOpen, file /builds/slave/tryserver-macosx-debug/build/db/sqlite3/src/sqlite3.c, line 27130.

this is on top of SQLite 3.7.1 on the try server.
Comment 18 Shawn Wilsher :sdwilsh 2010-08-24 09:51:19 PDT
Good news is that the assertion is just checking that "if CREATE is set, then READWRITE must also be set".

This is entirely fixable.
Comment 19 Shawn Wilsher :sdwilsh 2010-08-24 10:41:11 PDT
Created attachment 468718 [details] [diff] [review]
v2.3

This should fix the try server failure (pushing to verify now).  Changes are small, but probably a good idea to have it looked at one more time.
Comment 20 Shawn Wilsher :sdwilsh 2010-08-24 11:26:02 PDT
Due to feature freeze, this blocks b5 (adding new API)
Comment 21 Andrew Sutherland [:asuth] 2010-08-24 13:26:49 PDT
Comment on attachment 468718 [details] [diff] [review]
v2.3

You might want to update the doxygen block on the Connection constructor since it explicitly calls out the CREATE bit and you've moved where the forcing logic happens.
Comment 22 Shawn Wilsher :sdwilsh 2010-08-24 14:25:22 PDT
Still hitting that assertion, but that's because clone copies flags.  I'll post an interdiff, but I don't think I'll need review.
Comment 23 Shawn Wilsher :sdwilsh 2010-08-24 14:37:28 PDT
Created attachment 468812 [details] [diff] [review]
v2.3 to v2.4 interdiff
Comment 24 Shawn Wilsher :sdwilsh 2010-08-25 10:56:10 PDT
OK, not quite there yet...

Running the next test: test_clone_readonly
Assertion failed: (isCreate==0 || isReadWrite), function unixOpen, file /builds/slave/tryserver-macosx-debug/build/db/sqlite3/src/sqlite3.c, line 26096.
Comment 25 Shawn Wilsher :sdwilsh 2010-08-25 14:31:05 PDT
Comment on attachment 468812 [details] [diff] [review]
v2.3 to v2.4 interdiff

(In reply to comment #24)
> Running the next test: test_clone_readonly
> Assertion failed: (isCreate==0 || isReadWrite), function unixOpen, file
> /builds/slave/tryserver-macosx-debug/build/db/sqlite3/src/sqlite3.c, line
> 26096.

Largely because I'm dumb:
>+    // Turn off SQLITE_OPEN_CREATE.
>+    flags = (~SQLITE_OPEN_READWRITE & flags);

I added the same assertion locally in the Connection constructor, and it's now passing tests.  Removing the assertion for pushing, but his is now ready.
Comment 26 Shawn Wilsher :sdwilsh 2010-08-25 14:40:46 PDT
Created attachment 469197 [details] [diff] [review]
v2.5

Sorry for all the bugspam.  This one should be good.
Comment 27 Shawn Wilsher :sdwilsh 2010-08-27 14:36:31 PDT
http://hg.mozilla.org/mozilla-central/rev/bc95b9869d3f
Comment 28 Eric Shepherd [:sheppy] 2010-08-30 12:45:36 PDT
The comments in the IDL seem to be misleading; they refer to cloning the database, but this is just cloning the connection to the database, not the database itself, right?
Comment 29 Eric Shepherd [:sheppy] 2010-08-30 12:55:16 PDT
Now documented:

https://developer.mozilla.org/en/mozIStorageConnection#clone%28%29

And mentioned on Fx4 for developers.
Comment 30 Shawn Wilsher :sdwilsh 2010-08-30 13:26:51 PDT
(In reply to comment #28)
> The comments in the IDL seem to be misleading; they refer to cloning the
> database, but this is just cloning the connection to the database, not the
> database itself, right?
Yeah, "database" refers to a connection pretty much (if not) uniformly in storage docs.  Will try to be more clear in the future.

Note You need to log in before you can comment on or make changes to this bug.