Differential D21717
Remove history query from DiffusionRepositoryController as it is unused Authored by cspeckmim on Sep 4 2021, 5:55 AM. Tags None Referenced Files
Subscribers
Details The history query for the repository page isn't actually used to display any content. It looks like it was previously used to display the last user which modified a file however this looks to be removed in D21404. This removes the history query from happening as well as updates DiffusionBrowseTableView to remove the parameters for passing this information in, resulting in also updating DiffusionBrowseController to no longer need to put this information together. Refs T13666
Diff Detail
Event TimelineComment Actions This construction: foreach (new FutureIterator($futures) as $future) { // ... } ...resolves futures in parallel. These constructions, or any other similar construction, resolve the futures in series: foreach ($futures as $future) { $future->resolve(); } $future_1->resolve(); $future_2->resolve(); // ... etc ... So this change potentially imposes a significant performance penalty on the page, theoretically making it up to twice as slow. When implemented correctly, a Future should not throw until it is resolved (at least, for any kind of "result of trying to resolve the future" error), so the Iterator loop should never throw. Some older Futures were less careful about this, but I believe major futures were pulled in line after T11968. From cursory inspection, I think the behavior of ConduitClientFuture is correct, and I can't immediately get the iterator loop to throw an exception (see T13666#256232 for my test case and observed behavior). Upshot:
Everything else here looks great (removing the unused future and handles, etc), but since I can't figure out a reproduction case that lines up with the description in T13666 I'm not sure if the underlying issue is me not reading properly, conflation, some other bug, etc. Can you, e.g., give me a patch which makes the iterator loop throw or maybe show me a screenshot of the failure state? (A possible issue is that my local install is "clustered", while your test install may not be, but I don't think this should impact behavior unless I'm forgetting about something.) I'm also happy to accept a version of this change that does everything except remove the loop if you want to simplify concerns a bit and deal with whatever's going on there separately. Comment Actions
Does that mean the ConduitClientFuture doesn't begin execution until resolve() is called or some other internal mechanism triggered by iterating a FutureIterator? I will definitely add this back in. I think I jumped to conclusion that the exception was coming from the iterator loop as it looked like resolving of the futures elsewhere all happened in try/catch blocks which look to appropriately handle the separate content, and the error definitely seemed to be coming from the history query and not one of the others (I'll update with better repro steps).
It does look like PhabricatorRepository::newConduitFuture() might return a different type of future depending on where the repository is hosted, but I've only looked at that briefly right now. Comment Actions
Right: creating a future doesn't start it (it doesn't make any network connections, launch any subprocesses, etc). Technically, you can start a future explicitly by calling $future->start(), but there's little/no reason to ever do so. Even if we started futures on creation, the process is ultimately still single-threaded and we don't get any kind of I/O handling "for free" in PHP, so execution must pass into most Futures regularly to make meaningful progress on them: for example, if you just start() an HTTP future and don't return to it, it will probably get about as far as opening a network connection but not actually send any bytes over the wire, so it isn't really executing fully in parallel. Subprocess futures usually don't need as much upkeep as network futures, but they can still stall if the stderr buffer on the child process fills up and the parent process hasn't returned execution into the Future to read the buffer.
Yeah, pretty sure this is the right analysis and the root of the issue. Comment Actions
It seems obvious in hindsight especially considering that PHP is single-threaded. I made the assumption that they were started somehow, I think being used to handling futures in a Java codebase where APIs which create/return futures usually always schedule them on an executor as well. Comment Actions Sigh maybe update arcanist too. I ran arc lint before arc diff and it said no lints... Comment Actions I tested this out and it works properly, in that there's no noticeable difference between the behavior with this change and without but including the new future updates |