[Accepted] SDL 0339 - Replace `thread::Thread` implementation with `std::thread` from C++11

Question

[Accepted] SDL 0339 - Replace `thread::Thread` implementation with `std::thread` from C++11

theresalech opened this issue 4 years ago · 8 comments

Hello SDL community,

The review of the revised proposal "SDL 0339 - Replace thread::Thread implementation with std::thread from C++11" begins now and runs through August 24, 2021. The original review of this proposal took place July 14 - July 27, 2021. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0339-replace-pthread-implementation-with-threads-from-cpp11.md

Reviews are an important part of the SDL evolution process. All reviews should be sent to the associated Github issue at:

#1163

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of SDL. When writing your review, here are some questions you might want to answer in your review:

Is the problem being addressed significant enough to warrant a change to SDL?
Does this proposal fit well with the feel and direction of SDL?
If you have used competitors with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
Please state explicitly whether you believe that the proposal should be accepted into SDL.

More information about the SDL evolution process is available at

https://github.com/smartdevicelink/sdl_evolution/blob/master/process.md

Thank you,
Theresa Lech

Program Manager - Livio
theresa@livio.io

Answer 1 · 2021-07-20T15:24:58.000Z

Since this is just a refactoring of existing code without adding new features, there is no need to change a minor version of the library.

Refactoring code still at least requires a minimum version change. Refactors can sometimes call for a major version change. Removing the current thread implementation, delegate classes, and public methods would be a major version change to SDL Core.

The same changes should be introduced not only for the threads but also for the synchronization primitives like mutex, conditional variable, etc., where required.

Is this proposal suggesting to change more than the thread implementation? These other primitives are currently implemented using Boost per this proposal: https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0044-use-Boost-library.md

C++11 provides no equivalent to pthread_cancel(pthread_t thread). As an option to resolve this issue, we can add some cancellation_token to thread attributes which contains a sign that the thread has been cancelled.

Please include an example of how SDL core will implement stopping threads. The current implementation has a lot of layers so it is important to highlight how much will change. The proposal includes the creation of a thread, but does not include details to stopping a thread.

Pthreads provides control over the size of the stack of created threads; C++11 does not address this issue. std::thread::native_handle can be used if needed.

Please include an example for how std::thread::native_handle would be used to address this issue.

The current implementation of threads in SDL Core is abstracted in a way that if a system does not support pthreads a developer can implement their own thread class without changing all of the classes that inherit it. This proposal makes it so every class using threads must use the std::thread class.

I like the general idea of updating the thread implementation but I think there still needs to be a thread wrapper class so a developer could switch the thread implementation to boost or other library in case their system does not support c++11 std::thread.

Answer 2 · 2021-07-21T14:35:34.000Z

The Steering Committee voted to keep this proposal in review, so that the author can respond to the comments and the discussion can continue on the review issue.

Answer 3 · 2021-07-27T12:44:09.000Z

Seems need major version change to SDL Core then.

This proposal can be considered a subset of the above, but only affects the update of threads.
Synchronization primitives were mentioned because they are standardized in C++11 and will probably be used in some places together with std::thread.
But this proposal does not imply a total replacement of primitive synchronization.

Here is an simplified example of how SDL Сore will implement stopping threads:

/*   Cancellation Token Class  */

class CancellationToken {
public:
  CancellationToken() : _cancelled(false) {}
  operator bool() const { return !_cancelled; }
  void cancel() { _cancelled = true; }
private:
  std::atomic<bool> _cancelled;
};

/*  Thread Wrapper Class */

class ThreadWrapper {
protected:
  std::thread _thread;
  CancellationToken _token;

  void Reset() {
    if (!_thread.joinable())
      return;

    _token.cancel();
    _thread.join();
  }

public:
  ThreadWrapper() = default;

  template <typename Delegate, typename... Args>
  void Start(Delegate &&delegate, Args... args) {
    _thread = std::thread(delegate, args..., std::ref(_token));
  }

  void Stop() {
    Reset();
  }

  ~ThreadWrapper() { Reset(); }
};

/*    Test Class Using Thread Wrapper  */

class UsbHandler : public ThreadWrapper {
public:
  void Init() {
    Start([](CancellationToken &token) {
      while (token) {
        ....
      }
    });
  }
};

/*    Usage   */
void Test() {
  UsbHandler uh;
  uh.Init();

  std::this_thread::sleep_for(std::chrono::seconds(5));
  uh.Stop();
}

This item is incorrect. std::thread::native_handle is not applicable here.
Here we can say that the std::thread does not allow changing the stack size.

In our case seems we don't need to specify a stack size. Since the default stack size (1MB - 2MB) meets our requests:
from smartDeviceLink.ini ThreadStackSize = 20480 bytes
from protocol_handler_impl kStackSize = 131072 bytes

Since the current SDL Core code is already using C++11 (lambdas, auto, smart pointers, etc.), we can also unify the multi-threaded code with the tools presented in C++11 standard.
However, we still need to make a wrapper over the std::thread,
since in its pure form it does not implement the RAII concept and we need to take care of the correct termination of the thread in case of exceptions and other unforeseen circumstances.

Answer 4 · 2021-07-27T19:40:58.000Z

👍 Major version change should be mentioned as a revision.
Revision: Proposal should be updated to only mention threads. Remove this statement: The same changes should be introduced not only for the threads but also for the synchronization primitives like mutex, conditional variable, etc., where required.
Thank you for the example. This should be included in the proposal noted as an example.
Thank you. Statement regarding native_handle should be removed and the downside section should note that specifying stack size is not needed.
👍 The example in number 3 will also note the usage of a wrapper class around std::thread. No revision requested for this point.

Answer 5 · 2021-07-28T14:49:20.000Z

The Steering Committee voted to return the proposal for the revisions (Items 1 - 4) outlined in this comment.

Answer 6 · 2021-08-18T00:42:45.000Z

The author has updated this proposal based on the Steering Committee's agreed upon revisions, and the revised proposal is now in review until August 24, 2021.

The specific items that were updated since the last review can be found in this merged pull request: #1171

Answer 7 · 2021-08-25T13:37:56.000Z

The Steering Committee fully agreed to accept this proposal.

Answer 8 · 2021-08-25T17:57:06.000Z

Implementation Issue: smartdevicelink/sdl_core#3765