Re: Causes of session management problems in Plasma 5

[Andreas Hartmetz]

On Donnerstag, 26. November 2015 08:55:19 CET Thomas Lübking wrote:
> On Donnerstag, 26. November 2015 05:19:34 CEST, Nicolás Alvarez wrote:
> > What do you mean with "konsole asks"? Things like "You have
> > multiple tabs open, are you sure you want to quit?" and "You
> > have unsaved changes"?
> 
> Yes.
> 
> > If so, the scenario you describe is bad regardless of session
> > restoration.
> 
> Yes. Unfortunaltely.
> 
> > If konsole has to ask the user and the user has a
> > chance to say no and cancel the logout at that point, then
> > kwrite shouldn't have exited yet!
> 
> That's what the spec says, but the ksmserver change suggestion is
> about buggy clients that behave exactly this way.
> > If "app canceling logout" is a thing, then logout should feel
> > transactional to the user. Either logout happens and all apps
> > exit, or logout doesn't happen and nothing exits.
> 
> Yes. That's what the spec says ...
> 
> > I guess the implementation of that would be: all apps should be
> > given a chance to ask their questions and approve or stop the
> > logout
> That'S exactly what happens, but ...
> 
> > before *any* app exits.
> 
> We cannot stop process from doing stupid things, like exiting in the
> wrong moment.
> > This is how MS Windows works, by the way (or used to work).
> 
> No, it's how not buggy applications work on windows.
> 
> Cheers,
> Thomas

I was going to write pretty much the same things, Thomas was just faster 
:)
XSMP is a fairly well thought out spec although the full text, which I 
haven't read in its entirety, seems to be too long for what the spec is 
supposed to achieve. Logout being transactional is a big item in it.

Re: Causes of session management problems in Plasma 5

[Henry Miller]

I'll object to no interaction after logout. More than once I've asked "why is 
logout taking so long, Jumped to a terminal (not always konsole) to look.

 Also, on Windows (at least) a running terminal application will block logout 
so I may need to kill the application while in a logout context.  I'm not sure 
how this relates to konsole on other platforms, but it seems like the use case 
exists. 

On November 23, 2015 7:02:46 PM CST, Andreas Hartmetz  
wrote:
>Hello,
>
>As apparently one of the last users of session management, because I 
>shut down my computers about once every day, I run into problems about 
>as often as I log into a session that is supposed to be restored.
>The number one problem is Konsole just not restoring.
>So I took some time to investigate the problem. The result is that
>there 
>are several bugs that conspire to break session restore. It goes about 
>like this:
>
>- ksmserver (the session manager) sends clients the "SaveYourself" 
>  message and collects the responses. This works fine.
>- In Qt applications, this results in a call to 
>  QGuiApplicationPrivate::commitData(), which calls 
>  QApplicationPrivate::tryCloseAllWindows() after the part that sends
>  the SaveYourselfDone response to the session manager.
>  When QGuiApplication::quitOnLastWindowClosed() is true (the default),
>  this results in the application quitting.
>- ksmserver notices that (e.g.) konsole has terminated and purges it
>  from its internal data
>- ksmserver rounds up remaining processes, which at this point do not 
> include konsole, and saves their restore data. konsole thus has saved 
>  its state, but ksmserver forgot about it and doesn't remember to do
>  anything with konsole when restoring the session later.
>
>The two most obvious errors are thus:
>
>- QGuiApplicationPrivate::commitData() calling 
>  QApplicationPrivate::tryCloseAllWindows(), together with
>QGuiApplication::quitOnLastWindowClosed() being true by default. Quote 
>  from documentation of signal QGuiApplication::commitDataRequest():
>  "You should not exit the application within this signal. Instead, the
>  session manager may or may not do this afterwards, depending on the
>  context."
>  Note that it says session manager and afterwards, not QGuiApplication
>  and virtually immediately.
>
>- The session manager not "locking down" or better copying the list of
>  clients *while* logging out. This would arguably only help buggy
>  clients, but may still be a net positive.
>  Why copy the list? Logout may be canceled, so it is valuable to keep
>  the main client list updated for after logout cancellation.
>
>- Bonus: I've found that KMainWindowPrivate::init() calls 
>  QCoreApplication::setQuitLockEnabled(true), which is equivalent to
>  QGuiApplication::setQuitOnLastWindowClosed(true), which is either
>  redundant with the default or overrides something the user has
>  explicitly changed. I noticed this while trying to narrow down the 
>  problem with a call to 
>  QGuiApplication::setQuitOnLastWindowClosed(false) from konsole's
>  Application class. Which is not a good solution, but sufficient as a
>  proof of concept fix. That fix works as far as session save and 
>  restore are concerned.
>
>So now I wonder what the chances are to fix those bugs.
>
>- Make ksmserver more robust in the face of clients dying too early,
>  sure. I hope to get around to it Soon(TM).
>
>- Remove QCoreApplication::setQuitLockEnabled(true) from 
> KMainWindowPrivate::init() - seems like a good idea to me, objections?
>
>- Remove any window closing from QGuiApplicationPrivate::commitData() -
>  this is actually an old feature that was even modified in 2014 to
>  fix a problem on Windows(?!) - I guess that one is there to prevent 
>  interaction after session saving, but it's a very crude way to do 
>  that. IMO it would be better to do nothing, it would be even better 
>  to block user input and possibly I/O handling for the duration of 
>  logout and unblock them when logout is canceled.
>  Note: the Windows fix is about the method being expected to *kill*
> the application, which probably comes from a lack of knowledge about X
>  session management which is the main purpose of that method. Commit
>  9835a63dde06a1e5d2f in qtbase.
>
>I'd be grateful for any additional insight and / or historical 
>perspective.
>
>Cheers,
>Andreas

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: Causes of session management problems in Plasma 5

[Andreas Hartmetz]

That's fine with me. No action is much easier to implement than blocking 
of specific events ;)

On Dienstag, 24. November 2015 02:31:30 CET Henry Miller wrote:
> I'll object to no interaction after logout. More than once I've asked
> "why is logout taking so long, Jumped to a terminal (not always
> konsole) to look.
> 
>  Also, on Windows (at least) a running terminal application will block
> logout so I may need to kill the application while in a logout
> context.  I'm not sure how this relates to konsole on other
> platforms, but it seems like the use case exists.
> On November 23, 2015 7:02:46 PM CST, Andreas Hartmetz 
 wrote:
> >Hello,
> >
> >As apparently one of the last users of session management, because I
> >shut down my computers about once every day, I run into problems
> >about as often as I log into a session that is supposed to be
> >restored. The number one problem is Konsole just not restoring.
> >So I took some time to investigate the problem. The result is that
> >there
> >are several bugs that conspire to break session restore. It goes
> >about like this:
> >
> >- ksmserver (the session manager) sends clients the "SaveYourself"
> >
> >  message and collects the responses. This works fine.
> >
> >- In Qt applications, this results in a call to
> >
> >  QGuiApplicationPrivate::commitData(), which calls
> >  QApplicationPrivate::tryCloseAllWindows() after the part that sends
> >  the SaveYourselfDone response to the session manager.
> >  When QGuiApplication::quitOnLastWindowClosed() is true (the
> >  default),
> >  this results in the application quitting.
> >
> >- ksmserver notices that (e.g.) konsole has terminated and purges it
> >
> >  from its internal data
> >
> >- ksmserver rounds up remaining processes, which at this point do not
> >
> > include konsole, and saves their restore data. konsole thus has
> > saved
> > 
> >  its state, but ksmserver forgot about it and doesn't remember to do
> >  anything with konsole when restoring the session later.
> >
> >The two most obvious errors are thus:
> >
> >- QGuiApplicationPrivate::commitData() calling
> >
> >  QApplicationPrivate::tryCloseAllWindows(), together with
> >
> >QGuiApplication::quitOnLastWindowClosed() being true by default.
> >Quote>
> >  from documentation of signal QGuiApplication::commitDataRequest():
> >  "You should not exit the application within this signal. Instead,
> >  the
> >  session manager may or may not do this afterwards, depending on the
> >  context."
> >  Note that it says session manager and afterwards, not
> >  QGuiApplication
> >  and virtually immediately.
> >
> >- The session manager not "locking down" or better copying the list
> >of>
> >  clients *while* logging out. This would arguably only help buggy
> >  clients, but may still be a net positive.
> >  Why copy the list? Logout may be canceled, so it is valuable to
> >  keep
> >  the main client list updated for after logout cancellation.
> >
> >- Bonus: I've found that KMainWindowPrivate::init() calls
> >
> >  QCoreApplication::setQuitLockEnabled(true), which is equivalent to
> >  QGuiApplication::setQuitOnLastWindowClosed(true), which is either
> >  redundant with the default or overrides something the user has
> >  explicitly changed. I noticed this while trying to narrow down the
> >  problem with a call to
> >  QGuiApplication::setQuitOnLastWindowClosed(false) from konsole's
> >  Application class. Which is not a good solution, but sufficient as
> >  a
> >  proof of concept fix. That fix works as far as session save and
> >  restore are concerned.
> >
> >So now I wonder what the chances are to fix those bugs.
> >
> >- Make ksmserver more robust in the face of clients dying too early,
> >
> >  sure. I hope to get around to it Soon(TM).
> >
> >- Remove QCoreApplication::setQuitLockEnabled(true) from
> >
> > KMainWindowPrivate::init() - seems like a good idea to me,
> > objections?>
> >- Remove any window closing from QGuiApplicationPrivate::commitData()
> >->
> >  this is actually an old feature that was even modified in 2014 to
> >  fix a problem on Windows(?!) - I guess that one is there to prevent
> >  interaction after session saving, but it's a very crude way to do
> >  that. IMO it would be better to do nothing, it would be even better
> >  to block user input and possibly I/O handling for the duration of
> >  logout and unblock them when logout is canceled.
> >  Note: the Windows fix is about the method being expected to *kill*
> > 
> > the application, which probably comes from a lack of knowledge about
> > X> 
> >  session management which is the main purpose of that method. Commit
> >  9835a63dde06a1e5d2f in qtbase.
> >
> >I'd be grateful for any additional insight and / or historical
> >perspective.
> >
> >Cheers,
> >Andreas

Re: Causes of session management problems in Plasma 5

[Thomas Lübking]

On Dienstag, 24. November 2015 02:02:46 CEST, Andreas Hartmetz wrote:


- The session manager not "locking down" or better copying the list of
  clients *while* logging out. This would arguably only help buggy
  clients, but may still be a net positive.


It would falsely restore clients that do not want to be restored (maybe also 
because they've gotten some autostart entry)


  Why copy the list? Logout may be canceled, so it is valuable to keep
  the main client list updated for after logout cancellation.


So if I logout, kwrite exits, konsole asks, I cancel, I restart kwrite, I logout, kwrite 
exits, konsole asks, I cancel, I restart kwrite, ... I end up with n iterations of kwrite 
on the restorage? Nahhh the SM should restore what the user left behind on logout, 
not what he had running in the former session "somewhen" ;-)

Other than that, thanks and kudos for the Investigation and get QGuiApplication 
fixed itr. =)

Cheers,
Thomas

Re: Causes of session management problems in Plasma 5

[Andreas Hartmetz]

On Mittwoch, 25. November 2015 18:05:26 CET Thomas Lübking wrote:
> On Dienstag, 24. November 2015 02:02:46 CEST, Andreas Hartmetz wrote:
> > - The session manager not "locking down" or better copying the list
> > of> 
> >   clients *while* logging out. This would arguably only help buggy
> >   clients, but may still be a net positive.
> 
> It would falsely restore clients that do not want to be restored
> (maybe also because they've gotten some autostart entry)

No, I don't think so. Clients that don't want to be restored don't 
respond to "SaveYourself" with "SaveYourselfDone". More details below.

> >   Why copy the list? Logout may be canceled, so it is valuable to
> >   keep
> >   the main client list updated for after logout cancellation.
> 
> So if I logout, kwrite exits, konsole asks, I cancel, I restart
> kwrite, I logout, kwrite exits, konsole asks, I cancel, I restart
> kwrite, ... I end up with n iterations of kwrite on the restorage?
> Nahhh the SM should restore what the user left behind on logout,
> not what he had running in the former session "somewhen" ;-)
> 
I haven't explained all the details.
First, by copy I mean a temporary copy that is never merged back into 
the main list, it's kept around only to know which processes have 
already agreed to have their session saved and submitted the 
corresponding data.
The saved process states are saved in files in ~/.config/session. 
However, those files do nothing if ksmserver doesn't also *restart* the 
respective process, usually with the -session  command-line 
argument. ksmserver only saves the list of processes to restore when 
logout has been "confirmed", which means when nothing can cancel it 
anymore. The list of processes to restore is saved in ~/.config/
ksmserverrc.
ksmserver even has or can have enough data to be able to clean up stale, 
already saved session files when logout is canceled. Currently it leaks 
konsole session files...

The current problem is that some applications die between submitting 
their state to ksmserver (SaveYourselfDone message) and ksmserver saving 
the list of processes to restart (writing ksmserverrc). It is AFAICS 
safe to assume that an application that has submitted its session data 
is really supposed to be restored.

> Other than that, thanks and kudos for the Investigation and get
> QGuiApplication fixed itr. =)
> 
Thanks! Wish me luck :)
It's going to be a behavior change, you know :/

> Cheers,
> Thomas

Re: Causes of session management problems in Plasma 5

[Nicolás Alvarez]

> El 25 nov 2015, a las 14:05, Thomas Lübking  
> escribió:
> 
>> On Dienstag, 24. November 2015 02:02:46 CEST, Andreas Hartmetz wrote:
>>  Why copy the list? Logout may be canceled, so it is valuable to keep
>>  the main client list updated for after logout cancellation.
> 
> So if I logout, kwrite exits, konsole asks, I cancel, I restart kwrite, I 
> logout, kwrite exits, konsole asks, I cancel, I restart kwrite,

What do you mean with "konsole asks"? Things like "You have multiple tabs open, 
are you sure you want to quit?" and "You have unsaved changes"?

If so, the scenario you describe is bad regardless of session restoration. If 
konsole has to ask the user and the user has a chance to say no and cancel the 
logout at that point, then kwrite shouldn't have exited yet!

If "app canceling logout" is a thing, then logout should feel transactional to 
the user. Either logout happens and all apps exit, or logout doesn't happen and 
nothing exits. I guess the implementation of that would be: all apps should be 
given a chance to ask their questions and approve or stop the logout before 
*any* app exits. And if the system asks an app if it's okay to logout, and the 
app says yes, then when the "exit because we're logging out" command arrives, 
it's too late for the app to say no and it *has* to exit. This is how MS 
Windows works, by the way (or used to work).

-- 
Nicolás

Re: Causes of session management problems in Plasma 5

[Thomas Lübking]

On Donnerstag, 26. November 2015 05:19:34 CEST, Nicolás Alvarez wrote:

What do you mean with "konsole asks"? Things like "You have 
multiple tabs open, are you sure you want to quit?" and "You 
have unsaved changes"?


Yes.

If so, the scenario you describe is bad regardless of session 
restoration.


Yes. Unfortunaltely.

If konsole has to ask the user and the user has a 
chance to say no and cancel the logout at that point, then 
kwrite shouldn't have exited yet!


That's what the spec says, but the ksmserver change suggestion is about buggy 
clients that behave exactly this way.

If "app canceling logout" is a thing, then logout should feel 
transactional to the user. Either logout happens and all apps 
exit, or logout doesn't happen and nothing exits.


Yes. That's what the spec says ...


I guess the implementation of that would be: all apps should be given a 
chance to ask their questions and approve or stop the logout


That'S exactly what happens, but ...


before *any* app exits.


We cannot stop process from doing stupid things, like exiting in the wrong 
moment.


This is how MS Windows works, by the way (or used to work).

No, it's how not buggy applications work on windows.

Cheers,
Thomas

Re: Causes of session management problems in Plasma 5

[Thomas Lübking]

On Donnerstag, 26. November 2015 00:37:35 CEST, Andreas Hartmetz wrote:

First, by copy I mean a temporary copy that is never merged back into 
the main list, it's kept around only to know which processes have 
already agreed to have their session saved and submitted the 
corresponding data.


The current problem is that some applications die between submitting 
their state to ksmserver (SaveYourselfDone message) and ksmserver saving 
the list of processes to restart (writing ksmserverrc).


Ok, that's not a problem.

It is AFAICS 
safe to assume that an application that has submitted its session data 
is really supposed to be restored.


Agreed, and if this is only meant as temporary condition we won't restart a 
session history either. Given the async nature of IPC the cover-up sounds 
reasonable enough, yes.

Cheers,
Thomas

Causes of session management problems in Plasma 5

[Andreas Hartmetz]

Hello,

As apparently one of the last users of session management, because I 
shut down my computers about once every day, I run into problems about 
as often as I log into a session that is supposed to be restored.
The number one problem is Konsole just not restoring.
So I took some time to investigate the problem. The result is that there 
are several bugs that conspire to break session restore. It goes about 
like this:

- ksmserver (the session manager) sends clients the "SaveYourself" 
  message and collects the responses. This works fine.
- In Qt applications, this results in a call to 
  QGuiApplicationPrivate::commitData(), which calls 
  QApplicationPrivate::tryCloseAllWindows() after the part that sends
  the SaveYourselfDone response to the session manager.
  When QGuiApplication::quitOnLastWindowClosed() is true (the default),
  this results in the application quitting.
- ksmserver notices that (e.g.) konsole has terminated and purges it
  from its internal data
- ksmserver rounds up remaining processes, which at this point do not 
  include konsole, and saves their restore data. konsole thus has saved 
  its state, but ksmserver forgot about it and doesn't remember to do
  anything with konsole when restoring the session later.

The two most obvious errors are thus:

- QGuiApplicationPrivate::commitData() calling 
  QApplicationPrivate::tryCloseAllWindows(), together with
  QGuiApplication::quitOnLastWindowClosed() being true by default. Quote 
  from documentation of signal QGuiApplication::commitDataRequest():
  "You should not exit the application within this signal. Instead, the
  session manager may or may not do this afterwards, depending on the
  context."
  Note that it says session manager and afterwards, not QGuiApplication
  and virtually immediately.

- The session manager not "locking down" or better copying the list of
  clients *while* logging out. This would arguably only help buggy
  clients, but may still be a net positive.
  Why copy the list? Logout may be canceled, so it is valuable to keep
  the main client list updated for after logout cancellation.

- Bonus: I've found that KMainWindowPrivate::init() calls 
  QCoreApplication::setQuitLockEnabled(true), which is equivalent to
  QGuiApplication::setQuitOnLastWindowClosed(true), which is either
  redundant with the default or overrides something the user has
  explicitly changed. I noticed this while trying to narrow down the 
  problem with a call to 
  QGuiApplication::setQuitOnLastWindowClosed(false) from konsole's
  Application class. Which is not a good solution, but sufficient as a
  proof of concept fix. That fix works as far as session save and 
  restore are concerned.

So now I wonder what the chances are to fix those bugs.

- Make ksmserver more robust in the face of clients dying too early,
  sure. I hope to get around to it Soon(TM).

- Remove QCoreApplication::setQuitLockEnabled(true) from 
  KMainWindowPrivate::init() - seems like a good idea to me, objections?

- Remove any window closing from QGuiApplicationPrivate::commitData() -
  this is actually an old feature that was even modified in 2014 to
  fix a problem on Windows(?!) - I guess that one is there to prevent 
  interaction after session saving, but it's a very crude way to do 
  that. IMO it would be better to do nothing, it would be even better 
  to block user input and possibly I/O handling for the duration of 
  logout and unblock them when logout is canceled.
  Note: the Windows fix is about the method being expected to *kill*
  the application, which probably comes from a lack of knowledge about X
  session management which is the main purpose of that method. Commit
  9835a63dde06a1e5d2f in qtbase.

I'd be grateful for any additional insight and / or historical 
perspective.

Cheers,
Andreas