Page MenuHomePhabricator

Allow dashboard panels to detect rendering cycles and arrest stack overflows
ClosedPublic

Authored by epriestley on May 15 2014, 6:54 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Dec 24, 6:47 AM
Unknown Object (File)
Sun, Dec 8, 4:50 PM
Unknown Object (File)
Sat, Dec 7, 7:35 AM
Unknown Object (File)
Thu, Dec 5, 4:08 AM
Unknown Object (File)
Thu, Dec 5, 3:56 AM
Unknown Object (File)
Sun, Dec 1, 5:28 AM
Unknown Object (File)
Nov 27 2024, 6:23 AM
Unknown Object (File)
Nov 26 2024, 4:41 PM
Subscribers

Details

Summary

Ref T4986. Ref T4983. Panels will soon be able to contain other panels, either via Remarkup ({W1}) or maybe through new types of meta-panels.

Allow panels to detect that they are being rendered very deeply and/or within themselves.

Test Plan

Faked some errors, got failed panel renders. Since panels can't really contain other panels yet, this doesn't really have an impact.

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

epriestley retitled this revision from to Allow dashboard panels to detect rendering cycles and arrest stack overflows.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: btrahan.
btrahan edited edge metadata.
btrahan added inline comments.
src/applications/dashboard/engine/PhabricatorDashboardPanelRenderingEngine.php
18

I'd expect this to be some class that extends the Aphront Graph class with cycle detection. (To support panel A which contains panel B which contains panel C, etc.) You'd probably want to check the graph out during edits at the dashboard / panel level and otherwise not load it.

123

hahaha, tip jar time

src/applications/dashboard/engine/PhabricatorDashboardRenderingEngine.php
29

this will be an error i think

This revision is now accepted and ready to land.May 15 2014, 8:21 PM
src/applications/dashboard/engine/PhabricatorDashboardPanelRenderingEngine.php
18

I think we probably won't be able to guarantee this in general. For example, suppose you have a panel (W123) showing the 5 latest feed stories, and we update feed so it renders remarkup, then someone makes a comment with {W123} in it. We can't (and shouldn't) block the comment.

We could do this in a more narrow way, but I suspect there will be so many cases like this that making any effort isn't worth the complexity, and just failing gracefully is good enough without trying to prevent things from getting into this state.

src/applications/dashboard/engine/PhabricatorDashboardRenderingEngine.php
29

Haha, good catch. This was test code that I neglected to remove.

epriestley updated this revision to Diff 21727.

Closed by commit rP63acd90cefaf (authored by @epriestley).