Put an upper limit on UTF8 file sizes #9495

JeffryBooher · 2014-10-09T06:26:48Z

This is for #7223

Start the discussion at limiting files to 4MB and under before we refuse to load them
Display a message if we've reached the limit opening a file
Implemented in AppShellFIleSystem which is really the only place this could go without rearchitecting a lot of file system work but looking for feedback there too

redmunds · 2014-10-14T00:02:32Z

src/filesystem/impls/appshell/AppshellFileSystem.js

    var FILE_WATCHER_BATCH_TIMEOUT = 200;   // 200ms - granularity of file watcher changes

    /**
+     * @const
+     */
+    var FILE_READ_SIZE_LIMIT = 4 * 1024 * 1024; // 4MB


@JeffryBooher This may be a reasonable default to help people with slow systems from opening files that will freeze Brackets, but what about putting this in a preference for people with better hardware?

In this comment, a 3MB file that was sent in as causing slowdowns works fine on my system. I'm sure it has a lot to do with hardware specs, so it would be nice if I was not limited to opening 4MB files.

@redmunds I thought about doing that but then that maybe this would help promote splitting code into smaller files :)

@JeffryBooher If we want to enforce coding guidelines like that then maybe we should have different limits for different files types? I would only expect a .log or .json file to ever get that large.

@redmunds good point. That seems like a fair amount of work to detect the language at this low of a level so we will need to just limit it based on a pref. You think 4MB is still a good starting point?

4MB seems like a good starting point.

This is an absolute limit users can't get around, so I don't think we should be too conservative in the name of avoiding potential performance slowdowns. The goal in #7223 is just to eliminate flat-out freezes and crashes from insanely huge files. Brackets will happily open a 5 MB file on my machine, so the 4 MB limit seems too low.

The file size reported in #7223 was 2.2 GB, which may be a tad high to use as the limit :-) But I tried opening a 68 MB text file in Brackets and it also caused an out of memory crash. So maybe we should put the limit somewhere in the 10-20 MB range?

@peterflynn Did you see this comment where user is complaining about Brackets "freezing" with a 3MB file on a system with a (spinning) hard drive?

@redmunds That report seems a little jumbled -- one person reports a freeze with a 390 MB file, the other user reports "lag" (without saying whether it's during opening or later) and gives a 3 MB file as an example. Lag isn't what we wanted to guard against here, though. Also, I have a fairly slow non-SSD hard drive and opening a 5 MB file works fine on my machine.

Again, I think we should be fairly conservative with this limit since it's a hard blocker for users, and the main goal is just to avoid full native crashes -- not merely slowness or lag.

redmunds · 2014-10-14T00:15:58Z

I was able to get around the limit (when I was creating a test file) by opening a file that was less than the limit, then adding a bunch of test until it was over the limit, and then saving. Since file was already open Brackets did not warn me. I could even close and re-open file in same session (I'm guessing because it was cached). It wasn't until I shutdown and re-started Brackets until I was warned.

I know this is a quick & dirty solution but I just thought I'd mention it because it could be confusing that you're working on a file, then later you can't work on same file.

redmunds · 2014-10-14T00:45:11Z

Files over the limit are not searched when using Find in Files. I think this is good, and it will help performance, but there is no message that informs user. I certainly don't want a modal dialog popping up for every Find in Files, but I'm wondering if we should show "only once per project per session" info dialog (similar to dialog when there are > 30K files) warning user that at least one file was skipped because it's over the file limit?

Actually, once users get used to this feature, seeing this dialog for every session might be annoying -- maybe only show "once per project" ?

JeffryBooher · 2014-10-14T01:01:24Z

Could we do a warn once dialog?

peterflynn · 2014-10-14T01:39:40Z

src/filesystem/impls/appshell/AppshellFileSystem.js

@@ -365,38 +373,38 @@ define(function (require, exports, module) {
        // Execute the read and stat calls in parallel. Callback early if the
        // read call completes first with an error; otherwise wait for both
        // to finish.
-        var done = false, data, stat, err;
+        var stat, 
+            masterPromise = new $.Deferred();


The FileSystem code was written to avoid using Promises, so it would remain agnostic of which Promise library its client uses. I don't know if that's still a worthy goal, but note that this would be the first time we break that design principle... so we should be sure we're ok with it. Otherwise we should tweak this to use callbacks instead.

Yeah, I thought about that but chose to use promises to avoid spaghetti-logic to allow for a stat object to be passed in or read, failed or succeeded, > or < the maximum size allowed and a seemingly random completion order of callback code (hence the comment about running stat checking in parallell).

Who do I need to convince?

I think it can be pretty simple to just use callbacks here:

Change masterPromise.done(function() { to something like function afterStat() {

Change each masterPromise.resolve(); to afterStat();

Remove the masterPromise.reject();

Move afterStat() declaration up above this block of code, to avoid JSLint complaints about forward references

The reason the original code was so complex was because it was running the stat() and read() in parallel -- but because of the max size check this code is no longer doing that (hence my note saying that comment is no longer true). So it's actually a much more straightforward serial flow now.

JeffryBooher · 2014-10-14T04:35:19Z

Moving this to review since there has been some review on it

redmunds · 2014-10-14T14:19:21Z

@JeffryBooher

Could we do a warn once dialog?

That's probably sufficient. Especially if the default size is increased as suggested here.

le717 · 2014-10-14T18:32:24Z

Just a quick remark. I use Brackets almost daily, and while my own files are nowhere near 4MB, occasionally a large JPG or PNG image that is used (even for a short bit) may be used, so I'm concerned this might block that. Also, as @peterflynn said, this is a hard coded, absolute limit that cannot be bypassed, and 4MB could be limiting to many people. I'd rather this be a preference as @redmunds said, even if it is only a short-term resolution to a larger fix. A hard value as restricting as this one, even if it is raised (:+1:) is almost a step backwards for Brackets, and I don't want Brackets to receive the same kind of criticism Atom has on this same topic.

peterflynn · 2014-10-14T18:57:11Z

@redmunds @JeffryBooher I don't think we should do a warn-once dialog -- we should show a dialog each time we refuse to open a file, just like we do for non-UTF files. Otherwise Brackets could appear to silently no-op without explanation.

@le717 This won't affect viewing image files, since we don't read their contents into memory in the same way.

peterflynn · 2014-10-14T18:59:26Z

On the question of a preference: I don't think we should try to make this a pref at this point. The aim is to avoid out-of-memory crashes, and the V8 memory ceiling seems pretty constant regardless of the user's machine. So there'd be no point in individual users changing the setting since it would virtually guarantee a crash -- which is obviously even worse than an error dialog.

redmunds · 2014-10-14T19:32:07Z

I think we should make the limit a preference to hedge our bets that we picked the wrong size (too big or too small). Making a pref is a small amount of code and effort, so not much risk spending out time in the wrong place. Also, Chrome and V8 are moving targets -- what if the current "memory ceiling" that "seems constant" goes up significantly in the version of chrome?

redmunds · 2014-10-14T21:13:23Z

@peterflynn Yes, opening a file over the file limit should show a dialog each time.

The "warn once" dialog suggestion is an additional warning to users that 1 or more files that should have been searched for Find in Files were skipped due to exceeding the file size. Probably applies to Quick Open and Quick Edit, too.

peterflynn · 2014-10-15T00:26:58Z

@redmunds I still disagree about the preference... it seems dangerous to encourage users to play with a setting this delicate. It's not really a moving target because we control when we upgrade CEF. Making it a preference also means shifting dependencies around in a way that could be risky -- we've hit several issues recently with circular dependencies caused by adding dependencies on PreferencesManager, and this would not only add a circular reference but also potentially force prefs to be loaded much earlier (the FS impl is one of the first modules we load).

Re the dialog: sorry for misreading that! Seems like a good thing to do in conjunction with warning about binary/non-UTF files that were omitted, which we currently don't do either. Maybe that should be a spin-off bug? (#9310 seems tangentially related, but probably not close enough to repurpose).

JeffryBooher · 2014-10-15T21:16:03Z

@redmunds this is ready for another review

redmunds · 2014-10-16T19:12:13Z

src/file/FileUtils.js

@@ -541,4 +553,5 @@ define(function (require, exports, module) {
    exports.getSmartFileExtension          = getSmartFileExtension;
    exports.compareFilenames               = compareFilenames;
    exports.comparePaths                   = comparePaths;
+    exports.MAX_FILE_SIZE_MB     = MAX_FILE_SIZE_MB;


Align = with the others.

redmunds · 2014-10-16T19:17:40Z

@JeffryBooher Done with code review.

JeffryBooher · 2014-10-16T21:15:03Z

@redmunds I liked your idea (over IRC) to move to have 2 constants (one for machines and one for humans). All changes pushed.

Ready for re-review. Thanks!

JeffryBooher · 2014-10-17T19:56:40Z

Closing in Lieu of #9585

add a file size governor

13d0934

JeffryBooher changed the title ~~Put an upper limit on UTF8 files~~ Put an upper limit on UTF8 file sizes Oct 9, 2014

JeffryBooher added the Development label Oct 13, 2014

redmunds reviewed Oct 14, 2014
View reviewed changes

redmunds self-assigned this Oct 14, 2014

peterflynn reviewed Oct 14, 2014
View reviewed changes

JeffryBooher added Review and removed Development labels Oct 14, 2014

JeffryBooher added PR Triage Complete and removed Review labels Oct 14, 2014

Merge branch 'master' into jeff/LimitFileSize

f427d01

JeffryBooher added 2 commits October 14, 2014 13:53

rtc:remove promises, increase file limit, display more info in warning

2ddd7ce

code cleanup

37f3326

JeffryBooher added 2 commits October 14, 2014 14:24

more cleanup

3785d40

remove unused modules

410f78f

JeffryBooher mentioned this pull request Oct 15, 2014

Brackets crashes on Find in Project #9362

Closed

peterflynn mentioned this pull request Oct 15, 2014

Brackets becomes unresponsive when working with a large code base #9444

Closed

redmunds reviewed Oct 16, 2014
View reviewed changes

JeffryBooher added 3 commits October 16, 2014 13:42

rtc:@redmunds

2d918c2

32=>16mb

1fe5256

refractor machine and human data

ae977c3

Merge branch 'master' into jeff/LimitFileSize

a33b9e6

JeffryBooher mentioned this pull request Oct 17, 2014

Limit document file sizes to 16MB on open #9585

Merged

JeffryBooher closed this Oct 17, 2014

JeffryBooher deleted the jeff/LimitFileSize branch October 17, 2014 19:56

core-ai-bot mentioned this pull request Aug 30, 2021

[CLOSED] Put an upper limit on UTF8 file sizes brackets-archive/bracketsIssues#8482

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Put an upper limit on UTF8 file sizes #9495

Put an upper limit on UTF8 file sizes #9495

JeffryBooher commented Oct 9, 2014

redmunds Oct 14, 2014

JeffryBooher Oct 14, 2014

redmunds Oct 14, 2014

JeffryBooher Oct 14, 2014

redmunds Oct 14, 2014

peterflynn Oct 14, 2014

redmunds Oct 14, 2014

peterflynn Oct 14, 2014

redmunds commented Oct 14, 2014

redmunds commented Oct 14, 2014

JeffryBooher commented Oct 14, 2014

peterflynn Oct 14, 2014

JeffryBooher Oct 14, 2014

peterflynn Oct 14, 2014

JeffryBooher commented Oct 14, 2014

redmunds commented Oct 14, 2014

le717 commented Oct 14, 2014

peterflynn commented Oct 14, 2014

peterflynn commented Oct 14, 2014

redmunds commented Oct 14, 2014

redmunds commented Oct 14, 2014

peterflynn commented Oct 15, 2014

JeffryBooher commented Oct 15, 2014

redmunds Oct 16, 2014

redmunds commented Oct 16, 2014

JeffryBooher commented Oct 16, 2014

JeffryBooher commented Oct 17, 2014

Put an upper limit on UTF8 file sizes #9495

Put an upper limit on UTF8 file sizes #9495

Conversation

JeffryBooher commented Oct 9, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redmunds commented Oct 14, 2014

redmunds commented Oct 14, 2014

JeffryBooher commented Oct 14, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffryBooher commented Oct 14, 2014

redmunds commented Oct 14, 2014

le717 commented Oct 14, 2014

peterflynn commented Oct 14, 2014

peterflynn commented Oct 14, 2014

redmunds commented Oct 14, 2014

redmunds commented Oct 14, 2014

peterflynn commented Oct 15, 2014

JeffryBooher commented Oct 15, 2014

Choose a reason for hiding this comment

redmunds commented Oct 16, 2014

JeffryBooher commented Oct 16, 2014

JeffryBooher commented Oct 17, 2014