Added by Marco Kinski almost 5 years ago. Updated about 4 years ago.
0%
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 |
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?
using it with WServer::post is wrong or only the creation/destruction without holding the corresponding UpdateLock?
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.
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.
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!
I changed ApplicationEvent to use shared_ptr.
While looking into #7881 I had a regression reproducing this bug again.