Opened 9 years ago

Closed 6 years ago

#2289 closed defect (fixed)

sugar freezes when register widget is clicked (if Server is unresponsive)

Reported by: lfaraone Owned by: tomeu
Priority: Unspecified by Maintainer Milestone: Unspecified
Component: Sugar Version: Unspecified
Severity: Major Keywords: r?
Cc: dipankar, sascha, quozl, rgs Distribution/OS: Ubuntu
Bug Status: Unconfirmed

Description

Adapted from https://launchpad.net/bugs/617813:

To reproduce the bug follow these steps-

  1. Run sugar emulator.
  2. set the Jabber server to localhost
  3. start "nc -l 8080" in a terminal. (assuming your computer is not running services on 8080)
  4. Right click and click on register widget.

This issue occurs when a Jabber server exists but is frozen. That state is reproduced above.

Patch attached.

Attachments (2)

0001-Register-Bug-Solution-LP-bug-617813.patch (3.1 KB) - added by lfaraone 9 years ago.
0001-Register-widget-click-event-reflects-a-timeout-on-un.patch (2.5 KB) - added by dipankar 9 years ago.

Download all attachments as: .zip

Change History (12)

Changed 9 years ago by lfaraone

comment:1 Changed 9 years ago by lfaraone

  • Keywords r? added

comment:2 Changed 9 years ago by lfaraone

  • Cc dipankar added

comment:3 follow-up: Changed 9 years ago by sascha_silbe

If you post the patch to the mailing list (using git-send-email) you'll have a better chance if it getting reviewed soon (it will happen in Trac as well, but might take more time).

Some things I noticed while giving the patch a quick look:

  • subject is not really useful
  • description is missing
  • non-bugfix changes mixed in (what's the reason for the string import change anyway?)
  • change descriptions ("New class [...]") belong in the commit description, not the source code
  • please consider adding docstrings to the new classes (but only if you come up with something that's more descriptive than the name itself)
  • if all we're doing in TimeoutServerProxy is adding a parameter and we only have one trivial caller, why add a new class at all?
+    def make_connection(self, host):
+        host, extra_headers, x509 = self.get_host_info(host)
+        conn = TimeoutHTTP(host, timeout=self.timeout)
+        return conn

Please combine the last two lines. The variable name doesn't add any information as it should be clear from the method name that the return value will be a connection (and if it isn't clear enough, a docstring would be better).

While this the timeout a stopgap to prevent Sugar from hanging indefinitely, a proper fix will additionally need to make the registration asynchronous. It might be too late to ship the full fix in 0.90, though.

comment:4 in reply to: ↑ 3 Changed 9 years ago by tomeu

Replying to sascha_silbe:

While this the timeout a stopgap to prevent Sugar from hanging indefinitely, a proper fix will additionally need to make the registration asynchronous. It might be too late to ship the full fix in 0.90, though.

Not sure if this fix is safer than rewriting it to use gio, have been reading about the different hacks that people have used to overcome this xmlrpclib limitation and looks like they depend on non-API stuff that could (and does) get broken between releases.

I think using xmlrpclib.dumps and xmlrpclib.loads together with gio could be simple enough, we are already using gio for async http:

http://git.sugarlabs.org/projects/sugar/repos/mainline/blobs/master/extensions/cpsection/updater/backends/aslo.py

comment:5 follow-up: Changed 9 years ago by dipankar

  • Cc sascha added

The classes that we have added in the code are inter-dependent.
Removal of any of the classes from it will cause fault in program and it will not run.
To chalk out the dependency:

TimeoutServerProxy uses TimeoutTransport.
TimeoutTransport uses TimeoutHTTP.

I tried searching for them in the standard libraries for Python, but couldn't find them.
So, they cannot be removed from the code in any case.

Wish to ask you whether you have an alternative to the above implementation. If yes, please let me know. Thank you.

comment:6 in reply to: ↑ 5 Changed 9 years ago by sascha_silbe

Replying to dipankar:

The classes that we have added in the code are inter-dependent.
Removal of any of the classes from it will cause fault in program and it will not run.

What I meant was replacing

    server = TimeoutServerProxy(url)

with

    server = xmlrpclib.ServerProxy(url, transport=TimeoutTransport())

and dropping class TimeoutServerProxy.

comment:7 Changed 8 years ago by quozl

Proposed patch http://patchwork.sugarlabs.org/patch/260/ has two reviewed-by tags but no acked-by. See also #2432.

comment:8 Changed 8 years ago by quozl

  • Cc quozl added

comment:9 Changed 8 years ago by rgs

  • Cc rgs added

comment:10 Changed 6 years ago by dnarvaez

  • Resolution set to fixed
  • Status changed from new to closed

Looks like it was landed.

Note: See TracTickets for help on using tickets.