Remove session-based flood / loop detection since it performs poorly, and has unfixable race conditions

I agree, we triggered this just in general testing. The browser can handle it.

Patch attached to remove this.

Bump! Any chance this could get into the next beta?

This is also seriously inefficient for high-traffic sites - bootstrapping drupal and doing a database lookup is bad enough for handling redirects, we should definitely avoid database writes.

After the update to RC1, we're getting the message about infinite loop on every page. Could this expose another underlying bug? If not, I think it's time for this patch to be committed.

Edit: the error occurred only on pages with redirects, removing and re-adding the redirects fixes the notice.

After the update to RC1, we're getting the message about infinite loop on every page. Could this expose another underlying bug? If not, I think it's time for this patch to be committed.

I am also receiving the infinite loop error. Reverting back to redirect 7.x-1.0-beta4

Also getting this error on lots of pages after update to rc-1

There may be another underlying issue that has caused this issue. I guess it's the auto path that is added on updates.

For me it was a taxonomy term (taxonomy/term/52) with current alias tag/foo that also had a redirection from tag/foo to taxonomy/term/52 defined. The redirect module need to make sure that this is not possible and delete redirections aliases that point to the same url like the current alias.

BUG: This means redirection module redirects from tag/foo to tag/foo. A clear FAIL.

Workaround: Manual delete colliding aliases from redirects list at admin/config/search/redirect/list.

For the next release we also need an update hook to delete all the overriding redirects that will cause endless loops and bring the site down.

In http://api.drupal.org/api/drupal/includes%21path.inc/function/path_save/7 we can catch all alias changes with http://api.drupal.org/api/drupal/modules%21path%21path.api.php/function/... and delete the alias from the redirect table.

I would also suggest to remove these logic as the input/duplicate need to be blocked somewhat earlier or at least change to class error:

    // Mark redirects that override existing paths with a warning in the table.
    if (drupal_valid_path($redirect->source)) {
      $row['class'][] = 'warning';
      $row['title'] = t('This redirect overrides an existing internal path.');
    }

Found a node that had the current alias also as redirect to itself defined. This node was NOT listed in the redirect list as warning.

Marked #1793230: Infinite loops as duplicate

I'm getting this error after updating to RC1: "The webpage at webpage.com has resulted in too many redirects. Clearing your cookies for this site or allowing third-party cookies may fix the problem. If not, it is possibly a server configuration issue and not a problem with your computer."

This has happened due to the automatic redirect creation. On some pages, we changed the URL, and then changed it back. Therefore, it has a redirect that is pointing it to itself. Is there any way to check for this and delete/ignore the redirect? We have this on 100+ pages, so it's not workable to remove them manually.

Marked #1796596: Fix and prevent circular redirects as duplicate.

Upgrading to rc1 has caused infinite loops for me as well. Is there a patch for this issue?

The following mysql command seemed to fix the infinite loop issues for me, without breaking any URL aliases.

 DELETE r FROM redirect r INNER JOIN url_alias u ON r.source=u.alias AND r.redirect=u.source;

Marked #1799430: Too Many Redirects 310 Error when alias and redirect is same as duplicate.

Disabled Automatically create redirects when URL aliases are changed for now as several node revision updates cause loops. Editors don't understand this.

hi,

i get the same problem..i use rc1 ...so if i try to visit my blog post under ./blogs/anyuser i get the msg

Oops, looks like this request tried to create an infinite loop. We do not allow such things here. We are a professional website!

i mean this is a standart drupal path and has nothing to do with any redirection

We should change the message to "This is a malfunctioning and untested module. Don't use me if you have a professional site!" *G*

The flood detection makes sense, but it's just an error that pop's up that should never happen and the underlying issue is that there are duplicate redirects created for existing aliases. So if this would work properly, there is no need to remove the flood detection. Setting CNW to get this critical bug fixed.

Hass, then can you please file a new and separate issue, rather than continuing to derail what this issue was about?

There is a lot of details in this case and 3 duplicates linking to this case. No.

I don't see much rationale for flood / loop detection but *if* it is somehow determined that it's useful then it needs to be made optional, for reasons of performance on high-traffic sites specified in #4

Re-opened #1796596: Fix and prevent circular redirects as it is not a duplicate of this issue. I found it also terribly difficult to perform a copy-paste so that the details from this issue were also available in the other issue. </sarcasm>

sry ..but i love hass big mouth ...he does not know that he is not always right

@dave: try this copy paste with an iphone. Let me know when you completed this. *sarcasm*

@eule: i do not see anything in this case from #2+ where I'm wrong.

Anyone care to review the approach and patch in #3 at #1796596: Fix and prevent circular redirects?

For what it's worth, I can say that I applied the patch in #1 and it removed my loop errors.

Something is defintatly wrong with this module atm! I had this error: 'Oops, looks like this request tried to create an infinite loop. We do not allow such things here. We are a professional website!'.

I hoped fixing it by applying the patch, but after applying the patch I got a browser error too many redirects, so it was right! Afterwards I disabled the redirect module and all works fine now.

Conclusion: current module is certainly flawed. BTW I used 7.x-1.0-rc1 together with this patch.

@JGO: Try the latest patch in #1796596: Fix and prevent circular redirects. It aims to prevent a direct redirect to self, which I guess accounts for the majority of looping issues.

its interesting. loop detecting is triggered here if i do

site/foo node/2
foo node/2

i do not see no loop :) have updated the whole drupal+all contribs to latest versions

Can we please make this module UNAVAILABLE or CARRY A BIG WARNING IN RED at its download page?

Since my infinite loop occurred at my home page (mydomain.com) and I don't have shell access to run MYSQL commands, it looks like I have no other option but to nuke my entire Drupal installation and start from scratch recreating my pages - 100 hours of work, right down the crapper.

Or is there some other way I can fix this?

@ewenss,

Download phpMyAdmin and then upload its files to a subfolder of your Drupal installation on the server (e.g. a folder called "phpmyadmin" right below index.php would work). Then you should be able to connect to http://yoursite.example.com/phpmyadmin/index.php and you will be presented with a MySQL login page.

Enter the username and password for your MySQL database connection that's found in your settings.php file (sites/default/settings.php, or sites/mysite.example.com/settings.php) and that should let you into your database so you can delete the offending duplicate rows.

(one way if you are not using multiple languages is to run the query from #19 using the "SQL" tab).

You can back up your database before you start using the "Export" tab. There's lots of good online phpmyadmin tutorials. Or if you get stuck you can send me a private message through Drupal.org and I'll give you a few more hints. Don't throw that 100 hours of work away!

Or just go straight to your modules page and disable Redirect.

@ewenss, please don't post the same request in multiple related issues.

And let's keep code issues code issues and support forums support forums.

+1 for fixing the underlying problem and not masking it with this message.

If this message must remain, it should be an error message, not a status message. End users should not be seeing strange error messages that provide them no value.

@ewenss:
If you are running a drupal site where you have no database access at all, you have bigger issues than the redirect module.

Marked #1841432: Improve error and log messages when redirect loop detection triggered as a duplicate of this.

Also marked #1817976: Updating an entity with an URL alias that matches an existing redirect causes bad data and circular redirect as a duplicate of this. Below is some of the discussion from there that is relevant to how to do loop/flood detection and reporting:

#4
Posted by Eric_A on November 8, 2012 at 8:38pm
With new entities come new aliases. Existing entities have their aliases changed often enough when editors can't make up their mind. Content language changes at times and thus the alias.
I don't think it makes sense to boldly delete a redirect entity every time there's a (temporary) conflict. We just need to make sure to not proceed with the redirect at a moment in time when it clearly makes no sense.

(One of the challenges with the current system is that node redirects do not simply redirect to a hard coded node path. It redirects to whatever the currently active alias may be.)

#5
Posted by Eric_A on November 8, 2012 at 8:48pm
And another note on the proposed query. The options part should be taken into account as well. Those columns are serialized arrays, though. So I'm not a 100% convinced you will always catch them all. Only processing in PHP will.

#6
Posted by azinck on December 7, 2012 at 9:10pm
There are a number of ways collisions between aliases and redirects like this can occur. I'm not sure there's a blanket set of logic that we can apply for resolving such collisions and since the path-related hooks (like hook_path_update) are often the only reliable place to detect the collisions that's too late to give a user a chance to make a decision on how to resolve the problem.

What if, instead, we were to build a report to show alias and redirect collisions? We might also give provide some tools there for cleaning up the collisions. To build such a report we need to think through exactly what entails a collision. I started thinking that through but my head began to hurt so I'll leave it for another day :).

#7
Posted by azinck on December 7, 2012 at 10:09pm
A closely related discussion: #1288768: Conflict between an old redirect (with Redirect module) and a new automatic alias (with Pathauto).

#8
Posted by matglas86 on December 10, 2012 at 7:38pm
@azinck thats a good idea. Even if its just to get our head around the different ways collisions can happen. We can work out the different situations and test we could build to try and fix it.

The issue here is that this situation has occurred for me just renaming a page's title. Staff needs to know that if this happens, the webmaster needs to take corrective action. I suggest this be kept but the error message be made more informative, e.g., report the page address to the webmaster.

update remaining tasks

I've created the following patch that will at least correctly identify the error as such, and provide some more helpful information

It would be cool if the watchdog entry also contains the source and destination information.

I have to agree with that. The only loop detection the module should do is to prevent broken aliases being made in the first place. It would make more sense to have all the redirects of an entity pointing to the main URL and update them all when the main URL changes. Then just remove the redirect of which the source URL equals the entity's new main URL (if it exists).

Dave asked for critical issues in the queue. Setting priority.

I never *asked* for critical issues hass, so do not put words in my mouth EVER.

If the module is broken we name this critical.

We experienced the same issue and the message:

Oops, looks like this request tried to create an infinite loop. We do not allow such things here. We are a professional website!

was showing up on our public facing pages because we had some circular references between aliases and redirects. I know this was a site building issue but the fact this message shows to public users is unprofessional in itself; it's not the users fault that happened.

I had to run a query like the one in #19 but if you redirect camel-case URLs (e.g. /Products -> /products) that query may not work if you use a case-insensitive MySQL collation (which is default). You must do a string sensitive comparison in that case forcing one of your comparison operands to a case-sensitive collation:

delete r from redirect r inner join url_alias u on u.source = r.redirect and r.source = u.alias collate utf8_bin

#1796596: Fix and prevent circular redirects has a working patch (it handles existing aliases, and also includes tests), so marking this one as duplicate.

Ok, I created #2119157: Improve performance on high-traffic sites by making database writes optional for making flood detection optional.

clarify race problem

The patch from #1 didn't apply. That's why it failed testing.

This patch has correct line numbers and has been created using latest dev version which this issue is pointing to.

fenstrat, I'm reopening this issue since #1796596: Fix and prevent circular redirects is addressing the issue by trying to prevent circular references.
This issue is about discussing the removal of a mechanism that isn't foolproof. In other words grendzy has a valid point by saying "it's the client's job".

Also triggering the test bot for the previous patch in #57.

@Sneakyvv isn't this what is being achieved in #2119157: Improve performance on high-traffic sites by making database writes optional?

I submitted #2119157: Improve performance on high-traffic sites by making database writes optional because I was starting to give up hope that this "feature" would be removed, not because I think it should stay.

The arguments above seem pretty clear, for example, Drupal 7 has lazy sessions, so HTTP clients don't have the session cookie that would be required to make the redirect loop detection even work in the first place. If the client is logged in then there is a session cookie, but it's still pointless because browsers have their own built-in capability to detect redirect loops.

Ok having re-read this issue from the top I'm convinced. While I'm far from the authority on this I'm going to go out on a limb and mark this as RTBC.

Prevention of loops is covered by #1796596: Fix and prevent circular redirects. Removing session based detection is covered here.

Just adding a version to use with stable.

Agree with issue; got a page with 404ing images and keep getting this message (404ing images will be handled separately, but don't want the user seeing the message, etc.)

Committed #57 to 7.x-1.x. Thank you everyone for your help here.