Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes for logout hang #592

Merged
merged 8 commits into from Oct 31, 2015
Merged

fixes for logout hang #592

merged 8 commits into from Oct 31, 2015

Conversation

@quozl
Copy link
Contributor

quozl commented Oct 16, 2015

A series of fixes for failure to logout, restart or shutdown, when an activity is hung. See the individual commits for narrative detail.

To cause a logout failure alert, start Terminal, then start xclock or xterm, then click on the Terminal and press Ctrl-Z, this sends a SIGSTOP to the child process. Now switch to home view or control panel and try to logout, restart, or shutdown.

Other methods are to send SIGSTOP to any activity process.

Other test cases:

  • check there is a busy cursor during startup and it goes away once journal is ready,
  • check there is a busy cursor during shutdown, restart, or logout, if there is enough time to see it,
  • make an activity not respond, check the logout failure alert appears,
  • click cancel on the alert, switch to the hung activity, unhang it, resume working; the system shall not quit,
  • click cancel on the alert, switch to the hung activity, switch to home view, try logout again, click cancel, switch to hung activity, unhang it, resume working; the system shall not quit,
  • click continue on the alert; the system shall quit,
  • allow the timer to expire; the system shall quit.

Replaces #587

Requires sugarlabs/sugar-toolkit-gtk3#263 and sugarlabs/sugar-toolkit-gtk3#266.

References http://lists.sugarlabs.org/archive/sugar-devel/2015-October/050877.html

quozl added 8 commits Oct 15, 2015
Add busy cursor methods, with a reference counter.  Flush the cursor
change to the display.  Reduce the time between checks for journal
ready.

Why?

- the busy cursor effect was not re-entrant; if the busy cursor was
  already in use it would not be restored,

- a previous patch to add busy cursor on logout never worked properly
  because the cursor change was not flushed to the display,

- the busy cursor should be removed as soon as practical once the
  journal is ready.
Add a cancel shutdown method to the session manager.  Remove a delay
before final shutdown.

Why?

- a cancel method is needed by a later patch to handle hung activities,

- the one second delay before final shutdown is unnecessary, and makes
  Sugar feel unresponsive.
Add reference counted busy cursor methods, and flush the display.

Why?

- the busy cursor was delayed, because the change was not flushed,

- a busy method will be useful for other delays in the control panel.
Move the add and remove of alerts into new methods, for re-use.
Add a quit failure alert to the menu and the control panel.  After 30
seconds with no click, continue with the quit.

Why?

The buddy menu has options for shutdown, restart or logout.  The control
panel may also restart.  These make Sugar quit.  Session manager sends a
save-yourself message to running activities, and waits for the
activities to respond.

When an activity does not respond, Sugar does not quit.  No reason is
given.

Adding the alert will allow user to cancel the shutdown, cancel the
control panel section change, and work the problem, or lose unsaved
work.

Caution: requires a patch to

    sugar-toolkit-gtk3:src/sugar3/graphics/alert.py

otherwise a forced shutdown will occur 30 seconds after the alert is
shown, even if Cancel is clicked.
Remove previous stacked alerts implementation.  Remove alerts before
changing zoom level.  Fix button pressed callback to

No code was using alerts in the homewindow.  When it began to be used,
changes to zoom level were ruining the ordering of children of self._box
Use ErrorAlert so that the Ok button need not be added.

Connect before add to fit documented ordering.  No actual effect.
After a few seconds, force the session manager to shutdown if there
remain activities that are not responding.

	"just throw fire on everything and kill it anyway" -- Sam
@samdroid-apps

This comment has been minimized.

Copy link
Member

samdroid-apps commented Oct 16, 2015

This looks like a really good patch set. I have code reviewed it.

  • Did you finish writing the message of commit d5296ef?
  • Different time out in the control panel and in the home view (4 sec vs 3 sec)

Otherwise it looks really good.

I'm not 100% sure on the alert implementation. I think that we are suffering from the fact that there is no way to push a critical alert to users in sugar and having to implement the alert in 100 places. There was the same issue with the max running activities patch from memory - it only displays alerts in the journal and home view.

The previously proposed solution was to show the alerts in activities by adding a new dbus api or something. But this did not get merged because it added a new api and that wasn't well received.

This is something we seriously need a design discussion about, but the design discussion is separate from this patch, which does everything consistently!

@samdroid-apps

This comment has been minimized.

Copy link
Member

samdroid-apps commented Oct 16, 2015

  • Different time out in the control panel and in the home view (4 sec vs 3 sec)

Just to add a note - this should be a constant not a magic number too

@samdroid-apps

This comment has been minimized.

Copy link
Member

samdroid-apps commented Oct 16, 2015

And the 30sec timeout should probably have a constant as well, that's all 😄

@quozl

This comment has been minimized.

Copy link
Contributor Author

quozl commented Oct 16, 2015

Thanks for the review.

Did you finish writing the message of commit d5296ef?

You're right, there's a missing 'o.' on the end of the first paragraph, and a missing "." on the end of the last, but it is trivial, so please wait for others to review and me to agree before closing this pull request. I'd like to give everyone a chance to review.

Different time out in the control panel and in the home view (4 sec vs 3 sec)

Yes, because the control panel is invoked from the home view, and on an XO-1 the control panel steals more memory, causing any activities to take longer to respond to save-yourself message, since they will have to page fault to bring contents of virtual memory into physical memory.

I don't think this needs to be a comment or commit detail though.

should be a constant not a magic number too

I disagree, and would like other developers to give an opinion. @tchx84, @godiard? The problem with constants with only single point of use is that it takes longer for someone new to read the code and locate the constant declaration. I'm fine with constants that have multiple uses and are near to the code that uses them, but otherwise they waste valuable developer time. We want to be welcoming to new developers.

I'm not 100% sure on the alert implementation.

As you will have seen from your review, the implementation of alerts in Sugar is inconsistent. But beyond fixing the HomeWindow alert for this patch series, it didn't seem like a good time to embark on such a project.

I was happy to try to make the busy cursor more consistent, following the GSoC or GCI contributions, and this series does achieve some of that. But there are some more opportunities there as well.

@quozl

This comment has been minimized.

Copy link
Contributor Author

quozl commented Oct 27, 2015

@samdroid-apps, looks like @tchx84, @godiard, and others are busy. Will you merge the patch if I redo it with constants, or are my arguments above persuasive? If it needs to be redone, exactly which constants do you want moved away from where they are used, and what is the reason for the move?

@samdroid-apps

This comment has been minimized.

Copy link
Member

samdroid-apps commented Oct 27, 2015

Na looks fine. I'll merge it soon

On Tue, Oct 27, 2015, 11:41 AM James Cameron notifications@github.com
wrote:

@samdroid-apps https://github.com/samdroid-apps, looks like @tchx84
https://github.com/tchx84, @godiard https://github.com/godiard, and
others are busy. Will you merge the patch if I redo it with constants, or
are my arguments above persuasive? If it needs to be redone, exactly which
constants do you want moved away from where they are used, and what is the
reason for the move?


Reply to this email directly or view it on GitHub
#592 (comment).

@quozl

This comment has been minimized.

Copy link
Contributor Author

quozl commented Oct 27, 2015

thanks.

@quozl

This comment has been minimized.

Copy link
Contributor Author

quozl commented Oct 31, 2015

I've cherry-picked this for an OLPC fork.

@samdroid-apps samdroid-apps merged commit 85451d2 into sugarlabs:master Oct 31, 2015
@quozl quozl deleted the quozl:12813-f branch Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.