Page MenuHomePhabricator
Create Task
Maniphest T2850

When changeset rendering fails, render error inline with a retry option instead of as a popup dialog
Open, LowPublic
Actions

Description

Currently, when a changeset fails to render (e.g., because an underlying git command failed), we show a modal error dialog to the user. This is fairly disruptive, especially if several changesets fail (you get a stack of modal dialogs). This happens in Differential (revision detail view) and Diffusion (commit detail view). It can be reproduced by inserting an artificial exception into changeset rendering.

Instead, we should catch these exceptions and show them inline, with an option to retry (some exceptions are temporary or load-related and will succeed on retry). That is, instead of showing the diff normally, these changesets would just show an error:

Couldn't load this junk:
 
  blay blah yada yaada git derped sry bro

         [ Retry ]

Original Description

In a local install of Phabricator I have some big .so files (linux compiled library) that were erroneously put in the repository. Whenever I visit the page of one of these commit, I get an error window overlaid showing that the git diff process was killed due to an out of memory error. This also seem to cause some sort of performance problems on the server which are likely caused by this diff.

I know that .so files shouldn't be put in repositories, but it'd be nice too if Phabricator wouldn't try to make a diff out of it when it encounters one. Also, is there a configuration variable for files to exclude from diffs? I couldn't find one.

Here's what the error says:

Unhandled Exception ("CommandException")

Command failed with error #137!
COMMAND
(cd '/home/ubuntu/git/repo' && git diff -M -C --no-ext-diff --no-color --src-prefix=a/ --dst-prefix=b/ -U65535 '8d47d1dd481310467438f801643acb97a7326f27^' '8d47d1dd481310467438f801643acb97a7326f27' -- 'Server/libModel_d.so')

STDOUT
(empty)

STDERR
Killed

Event Timeline

michelf added a project: Diffusion.Mar 29 2013, 7:59 PM
michelf added a subscriber: michelf.
epriestley added a comment.Apr 2 2013, 8:21 PM

Under normal circumstances, Git should detect binary files automatically -- for example:

$ git diff -M -C --no-ext-diff --no-color --src-prefix=a/ --dst-prefix=b/ -U65535 4b825dc642cb6eb9a060e54bf8d69288fbee4904 -- webroot/rsrc/image/search.png 
diff --git a/webroot/rsrc/image/search.png b/webroot/rsrc/image/search.png
new file mode 100644
index 0000000..6b0cc17
Binary files /dev/null and b/webroot/rsrc/image/search.png differ

We handle various binaries properly on this install, too -- here are some examples:

So the current behavior generally works and produces desirable results (e.g., images are shown). I'm also not sure where the kill is coming from -- we have the ability to timeout these queries, but it looks like we don't actually set timeouts anywhere. Maybe it's getting OOM killed or something?

How large is "huge"?

epriestley added a subscriber: epriestley.Apr 2 2013, 8:21 PM
michelf added a comment.Apr 2 2013, 9:21 PM

The .so file is 75 Mb. Just checked and the content is unmistakably binary.

Ok, I assumed Phabricator was the one responsible for deciding how to diff files (binary or text), but it makes sense that git be in charge instead. By trying a diff on the command line git is very slow on this machine, probably because this is a Amazon EC2 Micro instance that seems to be throttled when doing too much work. Perhaps this in turn triggers a timeout and a kill.

... well, the server was restarted this weekend and seems to behave much better. I still get the error in Diffusion though, and while on one commit it's still a "Killed", on the other I get this:

fatal: Out of memory, malloc failed

I'm not sure what git is doing when diffing a big binary file, but it probably isn't an issue with Phabricator.

epriestley triaged this task as Low priority.Apr 7 2013, 1:14 PM

Some approaches we could take here:

We could use git cat-file -s to test the old/new sizes of the files before running git diff, and impose some arbitrary cutoff. However, this would slow everything down for everyone and benefit only diffing giant .so files on micro instances, so I think it's probably not worth it. I'm also not sure if it would actually work -- it is possible that git cat-file -s still depends on reading the file into memory.

One thing we could reasonably do is just improve the UI, so you get an inline error (with an option to retry) instead of a popup dialog. This would make the failure less disruptive, at least.

epriestley renamed this task from Failure to diff huge .so file that shouldn't be diffed anyway + bad performance to When changeset rendering fails, render error inline with a retry option instead of as a popup dialog.Apr 7 2013, 1:15 PM
epriestley updated the task description. (Show Details)Apr 7 2013, 1:18 PM
joshuaspence added a subscriber: joshuaspence.May 31 2015, 6:02 AM
chad changed the visibility from "All Users" to "Public (No Login Required)".Jul 3 2015, 5:21 AM
Phabricator · Built by Phacility