Merge lp://staging/~robertcarr/mir/session-transactions into lp://staging/~mir-team/mir/trunk

PASSED: Continuous integration, rev:857
http://jenkins.qa.ubuntu.com/job/mir-ci/1056/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1373
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1258
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/294
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/294/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/680

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1056/rebuild

+ // from a second thread before input focus is set.xs
Use emacs much?

I'd prefer a second opinion on using the transaction function over just using a lock for consistency with the other code, but otherwise it seems to make sense.

PASSED: Continuous integration, rev:858
http://jenkins.qa.ubuntu.com/job/mir-ci/1063/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1383
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1268
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/301
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/301/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1063/rebuild

I see the need, but don't like the solution. Maybe my next article should be "recursive_mutex considered evil"?

What we are lacking is a clear synchronization strategy in our data model. (C.f. "The case for a "SceneGraph"" on mir-devel.)

I'm tempted to hold my nose and "Approve" but will try to think of a cleaner solution first.

...having thought about it: doesn't the problem stem from not holding SessionManager::mutex when these actions occur?

void msh::SessionManager::close_session(std::shared_ptr<mf::Session> const& session)
{
    auto shell_session = std::dynamic_pointer_cast<Session>(session);

    session_listener->stopping(shell_session);

    app_container->remove_session(shell_session);

    std::unique_lock<std::mutex> lock(mutex);
    set_focus_to_locked(lock, focus_sequence->default_focus());
}

Note that the SessionManager::mutex isn't seized until after the remove_session() call.

inline void msh::SessionManager::set_focus_to_locked(std::unique_lock<std::mutex> const&, std::shared_ptr<Session> const& shell_session)
{
    auto old_focus = focus_application.lock();

    focus_application = shell_session;

    focus_setter->set_focus_to(shell_session);
    if (shell_session)
    {
        session_listener->focused(shell_session);
    }
    else
    {
        session_listener->unfocused();
    }
}

Note that the SessionManager::mutex isn't seized.

> inline void
> msh::SessionManager::set_focus_to_locked(std::unique_lock<std::mutex> const&,
> std::shared_ptr<Session> const& shell_session)
> {
> auto old_focus = focus_application.lock();
>
> focus_application = shell_session;
>
> focus_setter->set_focus_to(shell_session);
> if (shell_session)
> {
> session_listener->focused(shell_session);
> }
> else
> {
> session_listener->unfocused();
> }
> }
>
> Note that the SessionManager::mutex isn't seized.

Sorry, that last bit is a thinko.

Also, is there any need for DefaultSessionContainer::remove_session() to throw? It can (trivially) meet its post condition.

Holding the SessionManager mutex isn't enough. Not that close_surface isn't routed through the SessionManager which is where this race is triggered. The existing locking around close_session is asctually enough to prevent it.

DefaultSessionContainer::remove_session perhaps need not throw (though I think this usage is indicative of programmer error). Still though, that would leave the race around WindowHandleRepository.

More than those two, there is a race in every portion of the shell which has code like:

Step 1: Get surface from session
Step 2: Query properties from surface.

Step 2 can always throw as it stands and there is no reasonable way to get around it.

Does anyone have opinions?

OK, I'm beginning to see that the code is broken worse than I thought.

In any case, the problems arise from a poorly synchronized model - and part of that is that some locks are in the wrong place (i.e. in the controller).

Putting recursive locks into the controller seems like the wrong answer.

Sadly, fixing the model ("scenegraph") is a larger conversation.