Skip to content

Do not emit an event if it will cause a loop#607

Merged
d00rman merged 3 commits intowikimedia:masterfrom
Pchelolo:rerender_cause
May 24, 2016
Merged

Do not emit an event if it will cause a loop#607
d00rman merged 3 commits intowikimedia:masterfrom
Pchelolo:rerender_cause

Conversation

@Pchelolo
Copy link
Copy Markdown
Contributor

@Pchelolo Pchelolo commented May 9, 2016

After an outage we've discovered, that when change-prop is introduced, some bugs playing together might cause rerender loops in RESTBase, when change-prop serenaders some content, and it creates the same event that caused a rerender to happen.

This PR adds some protection against it - change-prop will add the x-rerender-reason header (name subject to a bike shed) to a RESTBase request, containing an original event URI (it's enough as RB only emits resource_change events). If we are about to emit the same event causing a cycle - log an error and drop the event.

Questions:

  1. What to use as an event hash. I think URI is enough, but suggestions are welcome
  2. This doesn't prevent multiple-step loops (event1 causes rerender1 causes event2 causes rerender2 causes event1). Do we want to support cases like that? As a generic way of preventing multi-step loops, we've had an idea to add something like an XFF header to the event meta to indicate the whole sequence of events that caused some event to fire. This could be useful for debugging as well as to prevent multi-step loops

PS: need to add this header in change-propagation.

cc @wikimedia/services

@gwicke
Copy link
Copy Markdown
Member

gwicke commented May 10, 2016

  1. What to use as an event hash. I think URI is enough, but suggestions are welcome

Perhaps {topic}:{url}, as in resource_change://en.wikipedia.org/v1/page/html/Foobar ?

  1. This doesn't prevent multiple-step loops (event1 causes rerender1 causes event2 causes rerender2 causes event1). Do we want to support cases like that? As a generic way of preventing multi-step loops, we've had an idea to add something like an XFF header to the event meta to indicate the whole sequence of events that caused some event to fire. This could be useful for debugging as well as to prevent multi-step loops

Yeah, I think this would be really useful. Even in the absence of loops, it's really useful to see where an event came from.

sys/events.js Outdated
// the same event, it will cause a rerender loop. So, log an error and skip
// an event.
var reRenderReason = req.headers && req.headers['x-rerender-reason']
|| hyper._rootReq && hyper._rootReq['x-rerender-reason'];
Copy link
Copy Markdown
Member

@gwicke gwicke May 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[minor, bikeshed, alternative]: x-triggers, x-triggered-by, x-event-path, or x-events?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for x-triggered-by

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

x-restbase-triggered-by?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we may want to make it independent of RESTBase. We don't want to re-emit an event for the same URI/topic combo regardless of the original source.

@Pchelolo
Copy link
Copy Markdown
Contributor Author

Updated to match the changes in https://github.com/wikimedia/change-propagation/pull/32

@coveralls
Copy link
Copy Markdown

coveralls commented May 18, 2016

Coverage Status

Coverage increased (+0.05%) to 93.589% when pulling e24b6e5 on Pchelolo:rerender_cause into e9b661f on wikimedia:master.

sys/events.js Outdated
})
.filter(function(event) { return !!event; });

// Change-propagation will set up the x-rerender-reason header, indicating
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

x-rerender-reason?

@d00rman
Copy link
Copy Markdown
Contributor

d00rman commented May 24, 2016

Minor nits, LGTM otherwise

@Pchelolo
Copy link
Copy Markdown
Contributor Author

@d00rman updated

@coveralls
Copy link
Copy Markdown

coveralls commented May 24, 2016

Coverage Status

Changes Unknown when pulling acffff4 on Pchelolo:rerender_cause into * on wikimedia:master*.

@d00rman d00rman merged commit acad4d3 into wikimedia:master May 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants