Use std::shared_ptr instead of raw pointer. #991

mgautierfr · 2023-08-23T14:57:58Z

Using raw pointer is problematic as nothing prevent a user to create
a "consumer" on a temporary object, delete the object and use the consumer.

Using a shared_ptr ensure us to the object is keep alive while a consumer
is using it.
But it prevent us to create plain instance on the stack.

Fix #988

codecov · 2023-08-23T15:01:18Z

Codecov Report

Attention: 44 lines in your changes are missing coverage. Please review.

Comparison is base (e13324f) 38.90% compared to head (9166b67) 38.92%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #991      +/-   ##
==========================================
+ Coverage   38.90%   38.92%   +0.02%     
==========================================
  Files          58       58              
  Lines        4000     3990      -10     
  Branches     2199     2201       +2     
==========================================
- Hits         1556     1553       -3     
+ Misses       1101     1090      -11     
- Partials     1343     1347       +4

Files	Coverage Δ
include/manager.h	`100.00% <100.00%> (ø)`
include/name_mapper.h	`100.00% <ø> (ø)`
include/search_renderer.h	`100.00% <ø> (ø)`
include/server.h	`100.00% <ø> (ø)`
src/library_dumper.h	`100.00% <ø> (ø)`
src/libxml_dumper.h	`0.00% <ø> (ø)`
src/server.cpp	`65.62% <100.00%> (-1.05%)`	⬇️
src/server/internalServer.h	`77.77% <ø> (ø)`
include/library.h	`94.73% <66.66%> (-5.27%)`	⬇️
src/name_mapper.cpp	`59.45% <50.00%> (ø)`
... and 5 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

veloman-yunkan

Using raw pointer is problematic as nothing prevent a user to create
a "consumer" on a temporary object, delete the object and use the consumer.

Using a shared_ptr ensure us to the object is keep alive while a consumer
is using it.

This is not 100% true. An std::shared_ptr can be created from a regular pointer using a custom deallocator. Thus changing the API only makes the requirements about the object lifespan more explicit but doesn't actually bring any guarantees with it. What it does is the programmer is relieved from explicit management of object lifetimes, however at the expense of some risk of memory leaks (since, unlike garbage collected languages, C++-with-exclusive-usage-of-std::shared_ptrs doesn't automatically take care of reference cycles).

IMHO, the current justification for this relatively big change is not compelling enough.

veloman-yunkan · 2023-08-26T08:23:35Z

test/library.cpp

- kiwix::Library lib;
+ std::shared_ptr<kiwix::Library> lib;


You could have avoided a lot of trivial changes below as follows:

std::shared_ptr<kiwix::Library> libShPtr; // the purpose of this data member is to avoid changes in old code during an API change kiwix::Library& lib; LibraryTest() : libShPtr(kiwix::Library::create()) , lib(*libShPtr) {}

veloman-yunkan · 2023-08-26T09:57:14Z

include/html_dumper.h

@@ -33,7 +33,7 @@ namespace kiwix
 class HTMLDumper : public LibraryDumper
 {
 public:
- HTMLDumper(const Library* library, const NameMapper* NameMapper);
+ HTMLDumper(ConstLibraryPtr library, std::shared_ptr<const NameMapper> NameMapper);


IMHO, HTMLDumper and OPDSDumper could be replaced with free functions - they were implemented as classes for reasons unrelated to the goals and principles of object oriented design. Now, having them as classes enables creating the dumper at one place and using it later but do we really need that functionality? Isn't it better to require that they are used as if they were free functions (i.e use and discard immediately after construction while ensuring that the Library and NameMapper objects stay unchanged during that time)?

mgautierfr · 2023-09-01T12:23:45Z

This is not 100% true. An std::shared_ptr can be created from a regular pointer using a custom deallocator. Thus changing the API only makes the requirements about the object lifespan more explicit but doesn't actually bring any guarantees with it.

Indeed. But moving the constructor private and allowing the creation of the library only through a shared_ptr limits a lot this possibility. It is still possible to create a shared_ptr with a custom deallocator from shared_ptr::get(), but here I would say that it is the inherent risk of using c++ and if user does this, it is on its fault, not us.

What it does is the programmer is relieved from explicit management of object lifetimes, however at the expense of some risk of memory leaks (since, unlike garbage collected languages, C++-with-exclusive-usage-of-std::shared_ptrs doesn't automatically take care of reference cycles).

To have a reference cycles, we need to have object stored in the library which have reference to the library itself. I don't think it is the case and we don't allow user to put custom object in the library.

veloman-yunkan · 2023-09-03T13:42:32Z

What it does is the programmer is relieved from explicit management of object lifetimes, however at the expense of some risk of memory leaks (since, unlike garbage collected languages, C++-with-exclusive-usage-of-std::shared_ptrs doesn't automatically take care of reference cycles).

To have a reference cycles, we need to have object stored in the library which have reference to the library itself. I don't think it is the case and we don't allow user to put custom object in the library.

That's correct and I never implied that it wasn't. My point was that std::shared_ptr is not a magic bullet, so it doesn't always make sense to spend effort on switching to it.

All those dumper were not used by any of our other projects. They are only used internally, either by `Library::writeToFile` or the server.

mgautierfr · 2023-09-19T14:52:34Z

New version of the PR.

Mostly the same strategy, but as agreed in #988 (comment):

Various dumpers have been moved in private API (None of ours project was actually using it, so no alternative provided)
Use shared_ptr for the server (as previous version of the PR)
For SearchRendered, we keep the class publicly accessible but we change the API to not store NameMapper nor Library.

veloman-yunkan

The second commit (the one titled "Use std::shared_ptr instead of raw pointer.") is too big. I would have made that change in several smaller steps.

veloman-yunkan · 2023-09-25T08:29:57Z

include/search_renderer.h

- std::string getHtml();
+ std::string getHtml(const NameMapper* mapper, const Library* library);

-  /**
+ /**
 * Generate the xml page with the resutls of the search.
+ *
+ * @param mapper The `NameMapper` to use to do the rendering.
+ * @param library The `Library` to use to look up book details for search results.
+ May be nullptr. In this case, bookName is not set in the rendered string.
+ * @return The xml string
 */
- std::string getXml();
+ std::string getXml(const NameMapper* mapper, const Library* library);

+ protected: // function
+ std::string renderTemplate(const std::string& tmpl_str, const NameMapper* mapper, const Library *library);


It's better to pass the name mapper and library objects by reference rather than by pointer thus meaning two things:

both are mandatory (a pointer can be NULL)

the called function doesn't store the address of those objects

I have changed const NameMapper* to const NameMapper&.
But as library can be nullptr, I have keep a raw pointer.

…ence. We want to be sure that `Library` actually exists when we modify it. While it is not a silver bullet (user can still create a shared_ptr on a raw pointer), making the `Manager` keep `shared_ptr` on the library help us a lot here.

Same as `Manager`, we want to be sure that `Library` actually exists when we use it.

mgautierfr · 2023-09-25T14:56:57Z

The second commit (the one titled "Use std::shared_ptr instead of raw pointer.") is too big. I would have made that change in several smaller steps.

Done. The commit is split in several ones.

One noticeable change is that now InternalServer do NOT keep a const (shared) pointer on the NameMapper (previous version of the PR was).
I feel bad to keeping a const pointer on something that can change (UpdatableNameMapper) even if we (server) don't change it itself.

kelson42 · 2023-09-29T14:40:53Z

@veloman-yunkan any feedback?

veloman-yunkan

LGTM with a minor comment (mainly intended to serve as a proof that I looked at the changes) -
the description of the commit titled "Make the Server keep a shared_ptr instead of a raw Library pointer." was obviously copy-pasted but not fully updated.

We want to be sure that `Library` actually exists when we use it. While it is not a silver bullet (user can still create a shared_ptr on a raw pointer), making the `Server` keep `shared_ptr` on the library help us a lot here.

…nter. Same as for `Library`, we want to be sure that the `NameMapper` actually exists when the server is using it.

The why of this mutex is in `Library` is a bit complex. It has been introduced in c2927ce when there was only `Library` and no `std::unique_ptr<Impl>`. As introducing the mutex imply implementing the move constructor, we have split all data in `LibraryBase` (and keep a default move constructor here) and add the mutex in `Library` (and implement a simple move constructor). Later, in 090c2fd, we have move the `LibraryBase` to `Library::Impl` (which should have been `Library::Data`). So at the end, `Library::Impl` is never moved. We can move the `mutex` in it and still simply implement move constructor for `Library`.

As we now always use Library through a shared_ptr, we can make `Library` not copiable and not movable. So we can move back the data in `Library`, we don't care about move constructor.

As we enforce the use of Library through a shared_ptr, let's simplify user life (and code) with new "type".

By moving the nameMapper/library arguments in `getHtml`/`getXml` we avoid any potential "use after free" of name mapper or library as they are not stored.

mgautierfr · 2023-10-05T15:47:38Z

Commit message rewritten. Time to merge !

mgautierfr requested a review from veloman-yunkan August 23, 2023 14:57

mgautierfr added a commit to kiwix/kiwix-tools that referenced this pull request Aug 23, 2023

Adapt to new api introduced in kiwix/libkiwix#991

f408069

mgautierfr added a commit to kiwix/kiwix-desktop that referenced this pull request Aug 23, 2023

Adapt to new api introduced in kiwix/libkiwix#991

97b86c2

mgautierfr added a commit to kiwix/java-libkiwix that referenced this pull request Aug 23, 2023

Adapt to new api introduced in kiwix/libkiwix#991

e5d3fff

mgautierfr force-pushed the no_raw_pointer branch from a85bfdf to ba825d4 Compare August 24, 2023 08:56

veloman-yunkan reviewed Aug 26, 2023

View reviewed changes

This was referenced Aug 27, 2023

Adapt to new api introduced in kiwix/libkiwix#991 kiwix/kiwix-tools#633

Merged

Shared pointer should be used where appropriate #988

Closed

Remove dumpers from the public API.

49e99e7

All those dumper were not used by any of our other projects. They are only used internally, either by `Library::writeToFile` or the server.

mgautierfr force-pushed the no_raw_pointer branch 2 times, most recently from 0042cd5 to 8906af5 Compare September 19, 2023 14:50

mgautierfr requested a review from veloman-yunkan September 19, 2023 14:52

veloman-yunkan reviewed Sep 25, 2023

View reviewed changes

mgautierfr added 3 commits September 25, 2023 16:28

Make the library creatable only within a shared_ptr.

c203e07

Make the UpdatableNameMapper keep a shared_ptr.

efcbf6e

Same as `Manager`, we want to be sure that `Library` actually exists when we use it.

mgautierfr force-pushed the no_raw_pointer branch from 8906af5 to b13d3f8 Compare September 25, 2023 14:53

mgautierfr requested a review from veloman-yunkan September 25, 2023 14:57

veloman-yunkan reviewed Sep 30, 2023

View reviewed changes

mgautierfr added 5 commits October 5, 2023 16:36

Make the Server keep a shared_ptr instead of a raw NameMapper poi…

1316dec

…nter. Same as for `Library`, we want to be sure that the `NameMapper` actually exists when the server is using it.

Make SearchRendered taking a const pointer.

ead1474

Move back Library::Impl in Library.

5292f06

As we now always use Library through a shared_ptr, we can make `Library` not copiable and not movable. So we can move back the data in `Library`, we don't care about move constructor.

mgautierfr added 2 commits October 5, 2023 16:37

Introduce LibraryPtr and ConstLibraryPtr.

1dc9705

As we enforce the use of Library through a shared_ptr, let's simplify user life (and code) with new "type".

Do not allow SearchRendered to work on a delete nameMapper/Library.

9166b67

By moving the nameMapper/library arguments in `getHtml`/`getXml` we avoid any potential "use after free" of name mapper or library as they are not stored.

mgautierfr force-pushed the no_raw_pointer branch from b13d3f8 to 9166b67 Compare October 5, 2023 14:38

mgautierfr merged commit e49abc1 into main Oct 5, 2023
13 checks passed

mgautierfr deleted the no_raw_pointer branch October 5, 2023 15:47

mgautierfr added a commit to kiwix/kiwix-desktop that referenced this pull request Oct 6, 2023

Adapt to new api introduced in kiwix/libkiwix#991

ab80463

mgautierfr mentioned this pull request Oct 6, 2023

Adapt to new api introduced in kiwix/libkiwix#991 kiwix/kiwix-desktop#992

Merged

mgautierfr added a commit to kiwix/kiwix-tools that referenced this pull request Oct 6, 2023

Adapt to new api introduced in kiwix/libkiwix#991

f7a4001

mgautierfr added a commit to kiwix/kiwix-tools that referenced this pull request Oct 6, 2023

fixup! Adapt to new api introduced in kiwix/libkiwix#991

b5c722b

mgautierfr added a commit to kiwix/kiwix-tools that referenced this pull request Oct 6, 2023

Adapt to new api introduced in kiwix/libkiwix#991

13d8333

mgautierfr added a commit to kiwix/kiwix-tools that referenced this pull request Oct 6, 2023

Adapt to new api introduced in kiwix/libkiwix#991

61055ef

mgautierfr added a commit to kiwix/kiwix-tools that referenced this pull request Oct 6, 2023

Adapt to new api introduced in kiwix/libkiwix#991

018df0d

kelson42 pushed a commit to kiwix/java-libkiwix that referenced this pull request Nov 10, 2023

Adapt to new api introduced in kiwix/libkiwix#991

cebffcf

kelson42 pushed a commit to kiwix/java-libkiwix that referenced this pull request Nov 20, 2023

Adapt to new api introduced in kiwix/libkiwix#991

242db41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use std::shared_ptr instead of raw pointer. #991

Use std::shared_ptr instead of raw pointer. #991

mgautierfr commented Aug 23, 2023

codecov bot commented Aug 23, 2023 •

edited

Loading

veloman-yunkan left a comment •

edited

Loading

veloman-yunkan Aug 26, 2023

veloman-yunkan Aug 26, 2023

mgautierfr commented Sep 1, 2023 •

edited by veloman-yunkan

Loading

veloman-yunkan commented Sep 3, 2023

mgautierfr commented Sep 19, 2023

veloman-yunkan left a comment

veloman-yunkan Sep 25, 2023

mgautierfr Sep 25, 2023

mgautierfr commented Sep 25, 2023

kelson42 commented Sep 29, 2023

veloman-yunkan left a comment

mgautierfr commented Oct 5, 2023

Use std::shared_ptr instead of raw pointer. #991

Use std::shared_ptr instead of raw pointer. #991

Conversation

mgautierfr commented Aug 23, 2023

codecov bot commented Aug 23, 2023 • edited Loading

Codecov Report

veloman-yunkan left a comment • edited Loading

Choose a reason for hiding this comment

veloman-yunkan Aug 26, 2023

Choose a reason for hiding this comment

veloman-yunkan Aug 26, 2023

Choose a reason for hiding this comment

mgautierfr commented Sep 1, 2023 • edited by veloman-yunkan Loading

veloman-yunkan commented Sep 3, 2023

mgautierfr commented Sep 19, 2023

veloman-yunkan left a comment

Choose a reason for hiding this comment

veloman-yunkan Sep 25, 2023

Choose a reason for hiding this comment

mgautierfr Sep 25, 2023

Choose a reason for hiding this comment

mgautierfr commented Sep 25, 2023

kelson42 commented Sep 29, 2023

veloman-yunkan left a comment

Choose a reason for hiding this comment

mgautierfr commented Oct 5, 2023

codecov bot commented Aug 23, 2023 •

edited

Loading

veloman-yunkan left a comment •

edited

Loading

mgautierfr commented Sep 1, 2023 •

edited by veloman-yunkan

Loading