From kde-panel-devel Sun Sep 11 18:40:27 2016 From: Christoph Cullmann Date: Sun, 11 Sep 2016 18:40:27 +0000 To: kde-panel-devel Subject: Re: Review Request 128890: Make e.g. Baloo::Query thread safe. Message-Id: <20160911184027.10006.86098 () mimi ! kde ! org> X-MARC-Message: https://marc.info/?l=kde-panel-devel&m=147361924201454 --===============1856200180735673633== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit > On Sept. 11, 2016, 6:32 p.m., Anthony Fieroni wrote: > > src/engine/database.cpp, line 128 > > > > > > It's needed *m_env = 0;* > > Or you can add method close to call it in all way. > > Christoph Cullmann wrote: > I don't understand the comment ;) > 1) m_env != null means valid atm, therefore I can call here close > 2) after this, the destructor is over, no need to null it, if you access the database object after/during destruction, all is lost. > > Anthony Fieroni wrote: > Missing m_env = 0 after closing call. It can lead crash it future use. Ah ;=) I see, I misread the line! Thanks - Christoph ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128890/#review99103 ----------------------------------------------------------- On Sept. 11, 2016, 6:27 p.m., Christoph Cullmann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128890/ > ----------------------------------------------------------- > > (Updated Sept. 11, 2016, 6:27 p.m.) > > > Review request for KDE Frameworks, Plasma and Boudhayan Gupta. > > > Repository: baloo > > > Description > ------- > > lmdb itself is thread safe (e.g. you can use the same env in multiple threads). > Unfortunately, the Baloo::Database itself not, as open() might race against other open calls (we have one unique db object in baloo). > > => add non-recursive mutex (recursive mutex not needed, one just must avoid to call isOpen() or path() inside open, that is done, else no unit test works). > > > Diffs > ----- > > src/engine/database.h e3bb175 > src/engine/database.cpp ec7ae2e > > Diff: https://git.reviewboard.kde.org/r/128890/diff/ > > > Testing > ------- > > Unit tests still work. > > > Thanks, > > Christoph Cullmann > > --===============1856200180735673633== MIME-Version: 1.0 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 7bit

This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128890/

On September 11th, 2016, 6:32 p.m. UTC, Anthony Fieroni wrote:
src/engine/database.cpp (Diff revision 1)
bool Database::open(OpenMode mode)
120
        mdb_env_close(m_env);
128
        mdb_env_close(m_env);
It's needed m_env = 0;
Or you can add method close to call it in all way.
On September 11th, 2016, 6:35 p.m. UTC, Christoph Cullmann wrote:
I don't understand the comment ;)
1) m_env != null means valid atm, therefore I can call here close
2) after this, the destructor is over, no need to null it, if you access the database object after/during destruction, all is lost.
On September 11th, 2016, 6:37 p.m. UTC, Anthony Fieroni wrote:
Missing m_env = 0 after closing call. It can lead crash it future use.

Ah ;=) I see, I misread the line!
Thanks

- Christoph

On September 11th, 2016, 6:27 p.m. UTC, Christoph Cullmann wrote:

Review request for KDE Frameworks, Plasma and Boudhayan Gupta.

By Christoph Cullmann.

Updated Sept. 11, 2016, 6:27 p.m.

Repository: baloo

Description

lmdb itself is thread safe (e.g. you can use the same env in multiple threads).
Unfortunately, the Baloo::Database itself not, as open() might race against other open calls (we have one unique db object in baloo).
=> add non-recursive mutex (recursive mutex not needed, one just must avoid to call isOpen() or path() inside open, that is done, else no unit test works).

Testing

Unit tests still work.

Diffs

src/engine/database.h (e3bb175)
src/engine/database.cpp (ec7ae2e)

--===============1856200180735673633==--