Project

General

Profile

Feature #11036

Thread safe smart (weak) pointer for WApplication

Added by Dries Mys about 1 month ago. Updated 26 days ago.

Status:
New
Priority:
Normal
Assignee:
-
Target version:
Start date:
10/31/2022
Due date:
% Done:

0%

Estimated time:

Description

Consider the following basic worker thread example:

Wt::WApplication* app = wApp;
Wt::observing_ptr<Wt::WText> widget = ...; // some widget used to provide feedback
std::thread([app, widget](){
  ; // perform time-consuming task that should finish, even if the user closes its browser

  Wt::WApplication::UpdateLock lock(app);
  if (lock && widget)
    widget->setText("set some feedback ..."); // provide (intermediate) feedback to the user (finish state, error information, ...)
}).detach();

This code is not safe, as app may be already destroyed when requesting the update lock, i.e. you are accessing a dangling pointer.

The official server push example avoids accessing a dangling pointer by ensuring the worker thread finishes before the application is destroyed. However, this is inconvenient if the worker thread should continue even after the browser is closed. Using this approach may even degrade server performance by exhausting the available threads to handle incoming requests as std::thread::join blocks the current thread until the worker thread finishes (which may take quite some time if the update lock is not often requested and no other cancel mechanism is implemented). So even the server push example may benefit from a smart WApplication pointer (see further), as it would allow to detach the worker thread instead of joining it at WApplication destruction.

I'm aware that using Wt::WServer::post is often preferred over grabbing the update lock manually. Disadvantage is that extra attention is needed to avoid copying observing_ptr objects without having the update lock. Off course, this can be resolved by encapsulating the observing_ptr inside another std::shared_ptr (as is done by Wt::WServer::schedule).

Preferred interface of the solution:

Exposing a kind of weak_ptr for WApplication would be useful to make the example thread-safe in the following way:

weak_ptr<Wt::WApplication> app = wApp;
Wt::observing_ptr<Wt::WText> widget = ...;
std::thread([app, widget](){
  ; // perform time-consuming task

  Wt::WApplication::UpdateLock lock(app); // lock should fail if object referenced by weak_ptr is already destroyed.
  if (lock && widget)
    widget->setText("set some feedback ...");
}).detach();

Is Wt::observing_ptr<WApplication> a solution? Not without making Wt::observing_ptr thread safe

Note that Wt::observing_ptr<WApplication> could not be used, as this would be a chicken or the egg dilemma, i.e. update lock is needed to check whether the observing ptr is valid, but the application referenced by the observing ptr is needed to take the update lock. Enhancing observing_ptr to allow thread safe copying (just like weak_ptr) may be a solution, but this may result in an undesirable performance degradation of observing_ptr.

Work-around / possible solution: std::weak_ptr<Wt::WebSession>

Note that the following work-around could already be used (but it uses the internal and non-documented Wt::WebSession object):

std::weak_ptr<Wt::WebSession> session = wApp->session()->weak_from_this();
Wt::observing_ptr<Wt::WText> widget = ...;
std::thread([session, widget](){
  ; // perform time-consuming task

  std::shared_ptr<Wt::WebSession> session2 = session.lock(); // safely check whether the session is already destructed
  if (!session2)
    return;

  Wt::WApplication::UpdateLock lock(session2->app());
  if (lock && widget)
    ; // provide (intermediate) feedback to the user (finish state, error information, ...)
}).detach();

So, a solution may be to update UpdateLock to accept a std::weak_ptr<Wt::WebSession> object or a wrapper around this object to avoid exposing implementation details.


Related issues

Related to Improvements #11049: Mitigate issues that may arise from changing the session idNew11/07/2022

Actions
#1

Updated by Roel Standaert about 1 month ago

There are of course ways you could do your own bookkeeping, like having your own thread-safe registry of applications (map from session id to application pointer), and then have these applications register/unregister themselves, so there is a workaround available, but I agree that it may be a bit silly to externally manage this when the WServer already knows which applications it has.

Another possibility could be to get the UpdateLock from the WServer, like some WServer::lock(const std::string& sessionId) that returns an UpdateLock (UpdateLock could be made move-only, like a unique_ptr). WServer::lock could possibly even take a raw application pointer, and check its registry to verify whether that pointer is valid.

Disadvantage is that extra attention is needed to avoid copying observing_ptr objects without having the update lock.

Wouldn't this disadvantage exist in either case?

#2

Updated by Dries Mys 30 days ago

It's true the user may solve the issue himself in one way or the other. Therefore it's a feature request, not a bug report.

WServer::lock approach seems fine to me too. I slightly prefer passing WApplication as argument to this new function or to provide a WApplication WServer::application(const std::string& sessionId) too to avoid the need for passing both WApplication object and a sessionId to the worker thread.

Concerning the copying of the observing_ptr: converting the example to using Wt::WServer::post would be:

const std::string& sessionId = wApp->sessionId();
Wt::observing_ptr<Wt::WText> widget = ...;
std::thread([sessionId, widget](){
  ; // perform time-consuming task

  Wt::WServer::instance()->post(sessionId, [widget](){ // copies observing_ptr without UpdateLock ==> NOT ALLOWED!
    widget->setText("set some feedback ...");
  });    
}).detach();

This straight forward translation to Wt::WServer::post seems "correct" for a Wt newbie, but actually performs an invalid copy of the observing_ptr outside. Off course one can solve this by wrapping the observing_ptr inside a std::shared_ptr before starting the worker thread.

#3

Updated by Dries Mys 30 days ago

Correction of my last sentence: ... an invalid copy of the observing_ptr outside/without the application lock. ....

#4

Updated by Roel Standaert 26 days ago

What I was thinking is that UpdateLock could have an application() function that returns a pointer to the WApplication. The current WApplication is always accessible through WApplication::instance(), I imagine the implementation of that UpdateLock::application() would involve calling WApplication::instance().

#5

Updated by Roel Standaert 26 days ago

  • Target version set to future
#6

Updated by Dries Mys 26 days ago

Roel Standaert wrote in #note-4:

What I was thinking is that UpdateLock could have an application() function that returns a pointer to the WApplication. The current WApplication is always accessible through WApplication::instance(), I imagine the implementation of that UpdateLock::application() would involve calling WApplication::instance().

Indeed, my mistake. For me, a new UpdateLock::application() member function is not required. So, the proposed WServer::lock(const std::string& sessionId) function would be just fine to solve this issue.

#7

Updated by Roel Standaert 26 days ago

The only issue that session ids still suffer from is that the session id of a WApplicaton can change (see WApplication::changeSessionId()). I'm thinking we should have a unique identifier for WApplications that doesn't change, e.g. splitting it up into a session id and a session token. The post(...) function would only need a session id, while the client-side code also requires a token.

#8

Updated by Roel Standaert 26 days ago

  • Related to Improvements #11049: Mitigate issues that may arise from changing the session id added
#9

Updated by Roel Standaert 26 days ago

I added the related issue #11049.

Also available in: Atom PDF