448476 - Make mozIStorageConnection threadsafe

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[Shawn Wilsher :sdwilsh]

People need to use it on more than one thread.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Pretty simple :)

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Shawn, I won't be able to get to this until next week.

Comment 3

[Shawn Wilsher :sdwilsh]

Comment on attachment 331805 [details] [diff] [review]
v1.0

Let's try myk then

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 331805 [details] [diff] [review]
v1.0

Unfortunately, I'm not qualified to review this, not being familiar with the facility being used to ensure thread safety.

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 331805 [details] [diff] [review]
v1.0

switching to robarnold - I'll have bent look at it post landing just to make sure of things.

Comment 6

[Rob Arnold [:robarnold]]

Comment on attachment 331805 [details] [diff] [review]
v1.0

You need to document what each mutex guards

Does Close need a lock around its "if(mProgressHandler)" block?

In SetProgressHandler, shouldn't the lock be acquired before *aOldHandler = mProgressHandler? aOldHandler may not be what the caller expects. ReleaseProgressHandler seems to do this correctly.

Comment 7

[Shawn Wilsher :sdwilsh]

(In reply to comment #6)
> Does Close need a lock around its "if(mProgressHandler)" block?
Reads should be atomic, but I think it's not a bad idea to do that to be safe

> In SetProgressHandler, shouldn't the lock be acquired before *aOldHandler =
> mProgressHandler? aOldHandler may not be what the caller expects.
Yeah, I think you are right there as well.

Comment 8

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

I'm not documenting the mutexes because given their names and where they are declared in the header file (directly above the variables in question, with white space grouping them with said variable), I think it would be redundant, and excessive documentation isn't needed (nor wanted).

Comment 9

[Rob Arnold [:robarnold]]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/66ab9c4075c4