Project

General

Profile

Actions

Bug #7617

closed

creation and destruction of @bindSafe@ functor is not thread safe

Added by Marco Kinski almost 4 years ago. Updated over 3 years ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Target version:
-
Start date:
06/23/2020
Due date:
% Done:

0%

Estimated time:

Description

When creating and destroying multiple background threads using bindSafe onto the same widget, the Wt::Core::Impl::observer_info::observers_ vector of that widget can get corrupted.

Wt: 4.3.1

boost: 1.73

MSVC: 16.6.1


Files

callstack-and-output.txt (4.26 KB) callstack-and-output.txt Marco Kinski, 06/23/2020 06:01 PM
wt-concurrency-failure.cpp (2.15 KB) wt-concurrency-failure.cpp Marco Kinski, 06/23/2020 06:01 PM
Actions #1

Updated by Roel Standaert almost 4 years ago

It wasn't actually made to be thread safe in the first place. I guess the documentation gave you the idea that it would be thread safe, though?

Actions #2

Updated by Marco Kinski almost 4 years ago

seems so.

Actions #3

Updated by Marco Kinski almost 4 years ago

using it with WServer::post is wrong or only the creation/destruction without holding the corresponding UpdateLock?

Actions #4

Updated by Roel Standaert almost 4 years ago

You can't use bindSafe or copy the resulting function object without holding the corresponding UpdateLock (you're copying finishcb on every post). I think you should be able to post something that uses bindSafe unless there's an extra copy of the callback somewhere in Boost Asio that I'm not aware of. I think the destruction should be fine, since the observable will first clear its observers (while holding the update lock), turning the destruction of the observing_ptr into a no-op. As I'm writing this I'm actually starting to realize that there's also the case where the observing_ptr is destroyed before the observable, and I'd have to check to make sure if that use is safe.

Note: the reference capture is probably fine in this case, but I think it's best to capture this instead of risking the capture of anything that will go out of scope.

Actions #5

Updated by Roel Standaert almost 4 years ago

As it is currently, it appears that an observing_ptr can indeed not be part of the function object that you pass to post() as a callback.

WebController::handleApplicationEvent calls WebSession::queueEvent without the update lock, and queueEvent causes a copy of the ApplicationEvent, which contains the callback, including the observing_ptr that bindSafe uses.

Actions #6

Updated by Roel Standaert almost 4 years ago

This actually stops being an issue when we use a std::shared_ptr for the ApplicationEvent. Maybe we should just do that, surely the overhead is acceptable. That doesn't change the fact that you do need to have the UpdateLock at the moment you create your observing_ptr (i.e. call bindSafe), though!

Actions #7

Updated by Roel Standaert almost 4 years ago

  • Status changed from New to Implemented @Emweb

I changed ApplicationEvent to use shared_ptr.

Actions #8

Updated by Roel Standaert almost 4 years ago

  • Status changed from Implemented @Emweb to Resolved
Actions #9

Updated by Roel Standaert over 3 years ago

  • Status changed from Resolved to Closed
Actions #10

Updated by Marco Kinski over 3 years ago

works for me, thanks.

Actions #11

Updated by Marco Kinski over 3 years ago

While looking into #7881 I had a regression reproducing this bug again.

Actions

Also available in: Atom PDF