GNOME Bugzilla – Bug 644491
Rygel should not browse recursively through the hierachy
Last modified: 2012-07-17 06:51:29 UTC
When a new external plugin comes up, Rygel starts to browse it recursively to get a map of all containers. This means that if, for instance, the external plugin is showing a full filesystem, Rygel makes it to browse it completely to get all the containers (ie. directories) it has. This is worse in the case of online services. Let's figure out a plugin that exposes the content of Jamendo in the following hierarchy Root | |--> Artists | |--> Albums | |--> Songs Means that Rygel makes the plugin to get all available artists, and for each of them, all available albums, everything when the plugin just comes up, without an user asking really for it. Now figure out with a service with thousands or millions of elements. This is not very healthy, and can lead to Jamendo (or any other service) to ban the plugin.
From what I see from the source it does that to hook up to the updated signal in each container.
Before we attempt to fix this, I need to know one thing: Does rygel-grilo (and grilo in turn) still need to browse a container recursively if I ask for the number of direct children under it? If so, we can't possibly fix this AFAICT. The best we can do is to delay the browsing until the first time client browses the root container, which is hardly helping the issue at hand.
(In reply to comment #2) > Before we attempt to fix this, I need to know one thing: Does rygel-grilo (and > grilo in turn) still need to browse a container recursively if I ask for the > number of direct children under it? No. If rygel-grilo (now known as grilo-mediaserver2) doesn't know the number of children, it "lies" assuming the maximum possible number. We (Zeeshan and I) already talked about it and we agreed that was the best option.
(In reply to comment #3) > (In reply to comment #2) > > Before we attempt to fix this, I need to know one thing: Does rygel-grilo (and > > grilo in turn) still need to browse a container recursively if I ask for the > > number of direct children under it? > > No. > > If rygel-grilo (now known as grilo-mediaserver2) doesn't know the number of > children, it "lies" assuming the maximum possible number. Good! > We (Zeeshan and I) already talked about it and we agreed that was the best > option. Yeah, I remembered the discussion but not if we concluded anything but now I do. Thanks! :)
(In reply to comment #3) > (In reply to comment #2) > > Before we attempt to fix this, I need to know one thing: Does rygel-grilo (and > > grilo in turn) still need to browse a container recursively if I ask for the > > number of direct children under it? > > No. > > If rygel-grilo (now known as grilo-mediaserver2) doesn't know the number of > children, it "lies" assuming the maximum possible number. Yes, but you need to update it once you know it. Otherwise you get in a really bad situation: Client requests 64 children of the root container. Grilo returns two (like Categories and common feeds for youtube) but TotalMatches will be INT_MAX. Every control-point out there will keep on requesting items forever since it's trying to get those INT_MAX children but only gets two.
(In reply to comment #5) > (In reply to comment #3) > > If rygel-grilo (now known as grilo-mediaserver2) doesn't know the number of > > children, it "lies" assuming the maximum possible number. > > Yes, but you need to update it once you know it. Otherwise you get in a really > bad situation: > > Client requests 64 children of the root container. Grilo returns two (like > Categories and common feeds for youtube) but TotalMatches will be INT_MAX. > Every control-point out there will keep on requesting items forever since it's > trying to get those INT_MAX children but only gets two. Good point!
Perhaps the solution to the 'child count not known' issue would to assume some sane number rather than MAXINT? Something like 128 or 256 should be more than enough. Yes, that would mean clients won't be able to browse everything under the container in many cases but we can live with that?
(In reply to comment #5) > > Client requests 64 children of the root container. Grilo returns two (like > Categories and common feeds for youtube) but TotalMatches will be INT_MAX. > Every control-point out there will keep on requesting items forever since it's > trying to get those INT_MAX children but only gets two. I don't get it completely..... If control-point requests 250 elements, but only two are returned, will it be continuing to requesting 250 even if they only get 2? I mean, will it enter in a loop asking elements from 2 to 250 even when it isn't getting elements? If so, truly they have a big problem, a bug I'd say. Else, I don't see the problem: they keep asking for MAXINT elements, and I'm only sending 2 all time. If control-point see that I only have 2 elements, where is exactly the problem?
(In reply to comment #7) > Perhaps the solution to the 'child count not known' issue would to assume some > sane number rather than MAXINT? Something like 128 or 256 should be more than > enough. Yes, that would mean clients won't be able to browse everything under > the container in many cases but we can live with that? In previous versions, grilo-mediaserver2 had an option to limit the maximum number of elements per folder. If that really fixes the problem (see my comment #6), then it wouldn't be a problem to reintroduce this option.
(In reply to comment #8) > (In reply to comment #5) > > > > Client requests 64 children of the root container. Grilo returns two (like > > Categories and common feeds for youtube) but TotalMatches will be INT_MAX. > > Every control-point out there will keep on requesting items forever since it's > > trying to get those INT_MAX children but only gets two. > > I don't get it completely..... > > If control-point requests 250 elements, but only two are returned, will it be > continuing to requesting 250 even if they only get 2? Yes. Because we say there's more. And we say that because grilo tells us that there is more. > > I mean, will it enter in a loop asking elements from 2 to 250 even when it > isn't getting elements? If so, truly they have a big problem, a bug I'd say. Nope. Totally conforming to the UPnP spec. And even if it's not a nice behavior, we can't fix all controlpoints out there. > > Else, I don't see the problem: they keep asking for MAXINT elements, and I'm > only sending 2 all time. If control-point see that I only have 2 elements, > where is exactly the problem? That the UPnP spec says otherwise. We could of course work around that in rygel, but that feels quite awkward architecture-wise. Wonder if using TotalMatch="0" as in "We've no idea" would be easier here.
(In reply to comment #10) > > If control-point requests 250 elements, but only two are returned, will it be > > continuing to requesting 250 even if they only get 2? > > Yes. Because we say there's more. And we say that because grilo tells us that > there is more. > Then, even if we adjust value to real after knowing it, still there's a problem in the first query, right? > > Wonder if using TotalMatch="0" as in "We've no idea" would be easier here. I think UPnP needs some kind of behaviour. One can not know for real the total elements in a source, or even if he can, can be extremely heavy to compute it.
(In reply to comment #11) > > > > Wonder if using TotalMatch="0" as in "We've no idea" would be easier here. > > I think UPnP needs some kind of behaviour. One can not know for real the total > elements in a source, or even if he can, can be extremely heavy to compute it. Yes, that's what I meant. Maybe we should translate children=INT_MAX on MediaServer2 side to TotalMatch=0 on UPnP side (that's supposed to mean: Don't know or cannot compute in timely manner). Just realized that while writing the previous comment.
Well, the other choice is allowing in the Mediaserver2 spec "I don't know how much children are in this folder". Something like returning "-1".
(In reply to comment #13) > Well, the other choice is allowing in the Mediaserver2 spec "I don't know how > much children are in this folder". > > Something like returning "-1". No, MAXINT is fine. Saves us a bit and chagning the MediaServer spec again.
Lets discuss this separate issue in the new bug I filed (bug#661988).
Can you check if "external" branch on https://git.gitorious.org/~phako/rygel/rygel-sandbox-phako.git fixes this?
(In reply to comment #16) > Can you check if "external" branch on > https://git.gitorious.org/~phako/rygel/rygel-sandbox-phako.git fixes this? I've ported MediaServer2 to use Grilo upstream, and added the MAX_INT as "unknown" children counter. Tested with the 'external' branch, and seems it works fine; I'm limiting the amount of data to 100 elements, and I can browse the content with Totem. I would say that this branch seems to truly fix the the problem. Good job!
Created attachment 218764 [details] [review] external: Don't recurse into external hierarchy
Created attachment 218765 [details] [review] external: Guard against empty mandatory properties Also do fingerpointing against the offending service.
Cleaned-up version of the aforementioned branch.
Attachment 218764 [details] pushed as e711eb7 - external: Don't recurse into external hierarchy Attachment 218765 [details] pushed as 95352d2 - external: Guard against empty mandatory properties