[patch] 2.3.3-kh6 fallback bug

[dadv]

Let's discuss following setup. We have first mount with max-listeners limited:

/nashe-192 1 /nashe-128.mp3 1

And we have fallback mount:

/nashe-128.mp3 5700 /nashe-64.mp3 1

When first mount is full, function source.c/source_add_listener() tries to find fallback mount for client, it founds it, updates "mount" string and continues with loop but forgets to update "minfo" structure.

For second loop iteration, if fallback mount is full too, it does not update "minfo" again, so loop would be endless without built-in loop breaker (10 iteration max) but this leads us to 403 error being sent to the user instead of serving it with next fallback.

Next, while checking for "max-listeners" limit this loop always compares current listeners count with original mount's limit instead of limit for fallback. So, in our case current listeners count is always compared with 1. This behavour differs from icecast-2.3.3 branch, is unexpected and undesirable. It often leads to non-working fallback when first mount has low limit.

The following patch fixes both problem for us and makes us allowed to use "internal mount redirects" like described above.

--- src/source.c.orig 2013-04-02 21:16:12.000000000 +0400 +++ src/source.c 2013-04-02 22:20:47.000000000 +0400 @@ -2022,7 +2022,7 @@ if (mountinfo->max_listeners == -1) break;

• if (source->listeners < (unsigned long)mountinfo->max_listeners)
• if (source->listeners < (unsigned long)minfo->max_listeners) break; INFO1 ("max listener count reached on %s", source->mount); } @@ -2031,6 +2031,7 @@ { thread_rwlock_unlock (&source->lock); mount = minfo->fallback_mount;
• minfo = config_find_mount (config_get_config_unlocked(), mount); INFO1 ("stream full trying %s", mount); loop--; continue;

[dadv]

Sorry, github's markup destroyed my examples and patch (my first attempt to use github at all). Here I fix it:

Let's discuss following setup. We have first mount with max-listeners limited:

<mount-name>/nashe-192</mount-name> <max-listeners>1</max-listeners> <fallback-mount>/nashe-128.mp3</fallback-mount> <fallback-when-full>1</fallback-when-full>

And we have fallback mount:

<mount-name>/nashe-128.mp3</mount-name> <max-listeners>5700</max-listeners> <fallback-mount>/nashe-64.mp3</fallback-mount> <fallback-when-full>1</fallback-when-full>

When first mount is full, function source.c/source_add_listener() tries to find fallback mount for client, it founds it, updates "mount" string and continues with loop but forgets to update "minfo" structure.

For second loop iteration, if fallback mount is full too, it does not update "minfo" again, so loop would be endless without built-in loop breaker (10 iteration max) but this leads us to 403 error being sent to the user instead of serving it with next fallback.

Next, while checking for "max-listeners" limit this loop always compares current listeners count with original mount's limit instead of limit for fallback. So, in our case current listeners count is always compared with 1. This behavour differs from icecast-2.3.3 branch, is unexpected and undesirable. It often leads to non-working fallback when first mount has low limit.

The following patch fixes both problem for us and makes us allowed to use "internal mount redirects" like described above.

[dadv]

--- src/source.c.orig   2013-04-02 21:16:12.000000000 +0400
+++ src/source.c        2013-04-02 22:20:47.000000000 +0400
@@ -2022,7 +2022,7 @@
             if (mountinfo->max_listeners == -1)
                 break;

—            if (source->listeners < (unsigned long)mountinfo->max_listeners)
+            if (source->listeners < (unsigned long)minfo->max_listeners)
                 break;
             INFO1 ("max listener count reached on %s", source->mount);
         }
@@ -2031,6 +2031,7 @@
         {
             thread_rwlock_unlock (&source->lock);
             mount = minfo->fallback_mount;
+            minfo = config_find_mount (config_get_config_unlocked(), mount);
             INFO1 ("stream full trying %s", mount);
             loop--;
             continue;

[karlheyes]

The example you state is a really bad example, you should always fall back or override to a stream/file with the same properties, a 192k stream is certainly not the same as 64k or 128k. Remember the fallback is primarily for moving listeners after the stream dies, not for mount selection at the beginning. URL auth could be used to allow a selection on the initial mountpoint and use the Mountpoint: header to move the rest to alternative mounts.

As for the code, the minfo is there for identifying the fallback mount if/when cascading, all other tests, like the authentication beforehand is against the originating requested mount. While the above change may be suitable for your set up, it is not generally and would need more work to allow for a completely backward compatible option to 2.3.x, probably involving a third variable that can be either depending on whether it's 2.3 compatible or not.

karl.

[dadv]

I understand and accept your criticism on bitrates. Let's assume we have /source1-128, /fallback1-128 and /fallback2-128 (all 128Kbps) instead of /nashe-192, /nashe-128.mp3 and /nashe-64. You see, bitrate is irrelevant to the problem I want to discuss.

The problem is 1) effectively endless loop in source_add_listener() that never gets to evaluation of /fallback2-128 and evaluates /fallback1-128 again and again until loop breaker hits zero value and 2) 403 error code returned to the user.

Now I see that correct patch should be more complex due to authorization problem you've mentioned (we run only public streams). Nevertheless, I still think that described configuration should not be interpreted as source of 403 errors. In 2.3.3 it has sensible interpretation and there is no reason for 2.3.3-kh branch to be incapable of dealing with it.

[karlheyes]

I am not entirely convince of your view that it's the only sensible interpretation, it's a valid one but not the only one. I suspect it's because of the view people have of a fallback, ie a fallback not supposed to be a main streaming point that others would use directly, after all, the listeners are not isolated when they fallback from others who connect directly.

If a fallback would become a hidden temporary stream which is a kind of relay, so that the listeners are isolated then matching the fallback mount details would be silly. There has been many cases where people want listeners isolated from the fallback, which is a common issue when people use a common fallback for many streams.

1. The endless loop you mentioned has a limit of 10 and the source changes each time, remember it's matching the requested mount against each source details.
2. The 403 will only occur if each source fails to match the criteria in the mountpoint requested, it's hardly a confusing interpretation.

It sounds like what you have is a difference in policy but I am unsure of what your goal is at the moment with regard to the streams you have and when you do and don't want to fallback. My initial guess was that you were doing some sort of first come, first served with 192k and 128k for the rest type of arrangement but I like to determine the direction of the code based on what people like to end up with.

karl.