added max file limit #16775

heejaechang · 2017-01-26T20:32:36Z

Customer scenario

User has big file (over 100MB) in solution and VS crashes OOM.

Bugs this fixes:

Workarounds, if any

remove the massive file from solution or split the file into smaller chunk

Risk

since we are now limiting size of text file (100MB) we will read, if someone had a big file which is big but not big enought to make VS OOM, that file will be no longer read into VS (Roslyn).

but we provide regkey to change default threshold, so we do still provide a workaround for such user.

Performance impact

there should be no change on performance

Is this a regression from a previous update?

No

Root cause analysis:

we need to read in whole file into memory to operate, we do chunk them so that it doesn't go to the LOH, but still we read whole thing in memory to operate rather than reading in or map portion of a file to operate. but naturally that sets limit on how much big file we can support, especially since devenv is in 32bit process. this fix explicitly set limit so that we reduce chance of going OOM.

since we at least chunk, we won't OOM due to lack of giant continuous memory block.

How was the bug found?

customer report.

Pilchie

I know it's for personal review only, but I couldn't resist, sorry...

Pilchie · 2017-01-26T20:38:08Z

src/Compilers/Core/Portable/FileSystem/FileUtilities.cs

+            }
+            catch (Exception e)
+            {
+                throw new IOException(e.Message, e);


Do you want to just use http://source.roslyn.io/Microsoft.CodeAnalysis/R/9c1e0a07e83f2ac3.html instead?

(Also, should that be changed to use an exception filter to avoid ever catching IOException in the first place)?

ah. ya. will change it to use that.

Why do we wrap in this way? My intuition is "just throw because catching non-Exception exceptions is trying to win a game of whack a mole".

We consistently distinguish between IOExceptions and anything else across the code base.

This change would argue that distinction is very questionable.

Yes, we are diligent and are wrapping them. It's worked well so far.

Catch where?

Sure. We are diligent to the places we know about and that's the problem. There are plenty of more cases where IO occurs that we don't know about. Every miss is just a crash that we're passing onto the customer instead of fixing now.

This is why I'm always very skeptical of approaches like this. Every time a customer reports the exception we will just catch ,wrap in IOException and move on. Hence the end behavior is indistinguishable from just catching Exception in the first place. This approach just causes us more work.

Yes, we need to be careful whenever we call outside of our code base. I'm not saying it's easy. But the solution is not to just catch and ignore all exceptions everywhere. That's a nightmare - things will stop working and nobody will know why. The customer won't be able to create a small repro and we won't be able to fix anything. So yes, you won't have VS crashing. But you won't have it working either. Better approach is to catch all exceptions in a filter with NFW reported on some recovery boundaries where we can report an error and continue running with no (best effort) state corruption.

@jaredpar @tmat so what is the conclusion? I am thinking not changing FileUtilities and take file length thing in to FileTextLoader itself.

I'd leave it here and do what we do in the rest of the FileUtilities - which is to turn non-IOExceptions to IOExceptions. It's consistent with the rest of the utils code.

Pilchie · 2017-01-26T20:38:26Z

src/Workspaces/Core/Desktop/Workspace/FileTextLoader.cs

+    internal static class FileTextLoaderOptions
+    {
+        [ExportOption]
+        internal static readonly Option<long> FileLengthThreshold = new Option<long>(nameof(FileTextLoaderOptions), nameof(FileLengthThreshold), defaultValue: 10 * 1024 * 1024,


Per mail-thread, I don't think this is large enough.

Pilchie · 2017-01-26T20:39:37Z

src/Workspaces/Core/Desktop/Workspace/FileTextLoader.cs

+            if (fileLength > workspace.Options.GetOption(FileTextLoaderOptions.FileLengthThreshold))
+            {
+                // log max file length to AI
+                Logger.Log(FunctionId.FileTextLoader_FileLengthThresholdExceeded, KeyValueLogMessage.Create(m => m["FileLength"] = fileLength));


Since this path will also be hit by AdditionalDocuments, is there something more actionable than just size that we can log here without capturing PII? File extension at least?

sure we can do that.

Pilchie · 2017-01-26T20:40:10Z

src/Workspaces/Core/Portable/WorkspacesResources.resx

@@ -610,4 +610,7 @@
  <data name="The_first_word_0_must_begin_with_a_lower_case_character" xml:space="preserve">
    <value>The first word, '{0}', must begin with a lower case character</value>
  </data>
+  <data name="File_size_exceeds_maximum_allowed_size_0" xml:space="preserve">
+    <value>File size exceeds maximum allowed size: {0}</value>


We're past loc cutoff, so it might be better to just use a hardcoded message in RTW, and update later. Not sure though.

jaredpar · 2017-01-27T16:15:44Z

src/Workspaces/Core/Desktop/Workspace/FileTextLoader.cs

+                Logger.Log(FunctionId.FileTextLoader_FileLengthThresholdExceeded, KeyValueLogMessage.Create(m => m["FileLength"] = fileLength));
+
+                var message = string.Format(WorkspacesResources.File_size_exceeds_maximum_allowed_size_0, path);
+                throw new IOException(message);


Why IOExecption? Why not just throw Execption. Or InvalidOperationException. No IO is actually occuring here.

+1 I don't think we should be throwing an IOException here. I'd either change it to Try-pattern, or define a new exception type that's gonna be handled here: http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces/Workspace/Solution/TextDocumentState.cs,177

@Pilchie how do you think. looks like FileTextLoader consider any exception happen inside FileTextLoader as IOException regardless they are generally considered IOException or not in OS standpoint.

heejaechang · 2017-01-27T19:13:27Z

ping?

heejaechang · 2017-01-27T19:17:31Z

@jaredpar @tmat @dotnet/roslyn-compiler @AlekseyTs @cston @VSadov can I get code review for changes in FileUtilities?

jaredpar · 2017-01-27T19:19:08Z

src/Workspaces/Core/Desktop/Workspace/FileTextLoader.cs

+        // File Threshold 100MB
+        [ExportOption]
+        internal static readonly Option<long> FileLengthThreshold = new Option<long>(nameof(FileTextLoaderOptions), nameof(FileLengthThreshold), defaultValue: 100 * 1024 * 1024,
+            storageLocations: new LocalUserProfileStorageLocation(@"Roslyn\Internal\Performance\Text\FileLengthThreshold"));


The use of "Internal" here seemed wrong given users may need to change it. Can't tell if this is an established pattern though. If it is then okay.

user doesn't change it through code, but through regkey, so code itself is internal.

Understood. I'm referring to the use of Internal in the reg key name.

ah, that one. ya, we put all reg key which is not public through tool->options, there.

jac009 · 2017-01-27T19:23:44Z

@heejaechang It's not clear to me that this has the telemetry of hitting the issue from my review. Is that just the "Log" statements?

AlekseyTs · 2017-01-27T19:29:47Z

FileUtilities LGTM.

heejaechang · 2017-01-27T19:30:42Z

@jac009 Yes. Log is the one that send telemetry to AI. since this code is in lower layer than VS layer, I can't use VS Telemetry API directly.

tmat · 2017-01-27T19:30:51Z

src/Workspaces/Core/Desktop/Workspace/FileTextLoader.cs

+            }
+        }
+
+        private static string TryGetFileExtension(string path)


Nit: I believe we already have a utility function to do this: PathUtilities.GetExtension

tmat · 2017-01-27T19:31:48Z

👍

heejaechang · 2017-01-27T22:43:25Z

@Pilchie should I ask matt for bar check for RTW?

heejaechang · 2017-01-27T23:03:02Z

@natidea just FYI, looks like we will bring this for RTW.

jasonmalinowski · 2017-01-30T20:29:13Z

src/Workspaces/Core/Desktop/Workspace/FileTextLoader.cs

+                Logger.Log(FunctionId.FileTextLoader_FileLengthThresholdExceeded, KeyValueLogMessage.Create(m =>
+                {
+                    m["FileLength"] = fileLength;
+                    m["Ext"] = PathUtilities.GetExtension(path);


Will this get associated with some other project type information? My worry is there's some customer out there with some large auto-generated file (think LINQ, EF, etc.) and we'll see here it's a C# file but not know further details. Although I'm not sure what else could be logged here...

@jasonmalinowski non-fatal watson then?

jasonmalinowski · 2017-01-30T20:31:03Z

src/Workspaces/Core/Portable/WorkspacesResources.resx

@@ -610,4 +610,7 @@
  <data name="The_first_word_0_must_begin_with_a_lower_case_character" xml:space="preserve">
    <value>The first word, '{0}', must begin with a lower case character</value>
  </data>
+  <data name="File_size_exceeds_maximum_allowed_size_0" xml:space="preserve">
+    <value>File size exceeds maximum allowed size: {0}</value>


Should this say what the file size was? Something like "File '{0}' size of {1} exceeds maximum allowed size of {2}"? Just so that way if a customer does hit that they have some hint what they should try getting their file under, vs. just going "uh, I need to make it smaller I guess?"

sure. @Pilchie who is localization champ by the way?

jasonmalinowski · 2017-01-30T20:33:31Z

Tagging @dotnet/roslyn-ide since this is touching workspaces.

CyrusNajmabadi · 2017-01-30T20:34:40Z

Yes. Please always tag @dotnet/roslyn-ide if we're making changes that touch infrastructural pieces.

CyrusNajmabadi · 2017-01-30T20:36:00Z

src/Compilers/Core/Portable/FileSystem/FileUtilities.cs

@@ -366,6 +366,29 @@ internal static DateTime GetFileTimeStamp(string fullPath)
            {
                return File.GetLastWriteTimeUtc(fullPath);
            }
+            catch (IOException)


We have other PerformIO helpers that handle all these sorts of cases. Is there a reason we're not using those?

I used FileUtilities since that was the one already used in this file. just looked PerformIO which seems very similar to FileUtilites, not sure why there is 2 utilities which does same thing.

opened tracking issue - #16836

Thanks #closed.

CyrusNajmabadi · 2017-01-30T20:37:03Z

src/Workspaces/Core/Desktop/Workspace/FileTextLoader.cs

+    {
+        // File Threshold 100MB
+        [ExportOption]
+        internal static readonly Option<long> FileLengthThreshold = new Option<long>(nameof(FileTextLoaderOptions), nameof(FileLengthThreshold), defaultValue: 100 * 1024 * 1024,


I don't know what FileLengthThreshold means. Can we comment this. What happens at 100MB? Do we simply ignore hte file entirely? Do we have the document, but we truncate it at 100MB? Do we have the document, but treat its contents as empty?

CyrusNajmabadi · 2017-01-30T20:37:38Z

src/Workspaces/Core/Desktop/Workspace/FileTextLoader.cs

+            var fileLength = FileUtilities.GetFileLength(path);
+            if (fileLength > workspace.Options.GetOption(FileTextLoaderOptions.FileLengthThreshold))
+            {
+                // log max file length to AI


What is AI?

application insight

Please don't abbr unnecessarily.

CyrusNajmabadi · 2017-01-30T20:38:16Z

src/Workspaces/Core/Desktop/Workspace/FileTextLoader.cs

+                }));
+
+                var message = string.Format(WorkspacesResources.File_size_exceeds_maximum_allowed_size_0, path);
+                throw new InvalidDataException(message);


How will this manifest higher up the stack? What is the end user experience this causes?

same as other failure in this file. showing in workspace failure output pane.

Please comment code to make that clear. It's also unclear to me what the user does at that point. Is the workspace unusable? Do they have any recourse on what they can do?

CyrusNajmabadi

I'm sorry, but i don't know what this change actually does. The code (and PR) need explanations of what's going on here and why it's the appropriate "fix" for whatever issue we're encountering.

heejaechang · 2017-01-30T20:43:11Z

@CyrusNajmabadi have you read description?

CyrusNajmabadi · 2017-01-30T20:44:14Z

Yes. THe description is extremely vague and unclear.

heejaechang · 2017-01-30T20:50:21Z

@CyrusNajmabadi you should ask @Pilchie and @MattGertz why it is appropriate fix since they are the one decide to do this.

heejaechang · 2017-01-30T20:51:50Z

@CyrusNajmabadi hmm.. so want me to add comment on which part? on 100MB limit or what happen it throws?

CyrusNajmabadi · 2017-01-30T20:52:03Z

I'm asking for the description (and code) to actually explain what is going on. Reading the code, i cannot tell what the actual behavior of tihs will be. The code is inadequately commented, and i cannot sign off that this is any sort of appropriate fix.

Please:

State what behavior we have now.
State what behavior we want to have.
Explain how this change produces the behavior we want.

heejaechang · 2017-01-30T20:53:15Z

@CyrusNajmabadi in description or in code? where in commit description?

MattGertz · 2017-01-30T20:53:47Z

Actually, it was JoC 's request...

Pilchie · 2017-01-30T20:55:09Z

Code is better than commit description, since it's right there later.

CyrusNajmabadi · 2017-01-30T20:56:46Z

in description or in code

Both are fine. but, at the very least, this needs to be in the code.

where

Ideally in any codepath that performs these checks. People reading the code need to understand what the checks are doing and what impact it has on the workspace. For example (as i asked earlier):

Is the workspace usable after this?
What impact does this have in the document in the workspace? i.e. is the document in the workspace? Is it there with truncated text? Is it there with empty text? etc. etc.
What options does the user have at this point? Do they need to close the project? Do they need to close VS? etc. etc.

Basically, nothing has actually been explained here. I want to know precisely what the impact of this change is; what we want to accomplish; and how this PR gets us that.

--

Note: this PR may be perfect (as far as the code change goes). I'm simply stating that i have no way to effectively review this change as i do not know what you're trying to accomplish, and thus i have no idea if your code change is appropriate.

heejaechang · 2017-01-30T21:02:19Z

@CyrusNajmabadi sure, trying to add comment. just to want to make sure where you want. since it is just throwing, it is other part that decide what to do with the caught exception. so, if I put comment what workspace does because of this exception in the code area I changed, it is far part from where it actually happens. so, if later some body change that behavior, that person will have no idea, the comment exist here which describe workspace behavior he is changing.

basically this code has nothing to do with 1,2,3. it is the one who caught the exception decide 1,2,3 which is in DocumentState I believe.

CyrusNajmabadi · 2017-01-30T21:06:52Z

Sorry, i cannot sign off without a complete understanding of the impact of this change. It is not sufficient ot say (effectively) "i am throwing, and it is someone else's responsibility as to what happens then". I need the actual information as to what impact this change will have. Otherwise, taking this fix would be very irresponsible as it could cause any number of bad things to happen, right when we're in escrow.

As for the comment location. If you feel like there are two distant piece of code that can be impacted by this, then feel free to comment both locations. i.e. in this location that you're changing, comment on which parts of the workspace that are affected by this. And in the workspace, please comment on which parts have to deal with the potential exceptions that can be thrown here.

that person will have no idea

If there is ever a concern about some 'person' having 'no idea' about what's happening, then the onus is on us to effectively comment hte places of concern so that that doesn't happen.

heejaechang · 2017-01-30T23:19:48Z

@CyrusNajmabadi updated comments!

CyrusNajmabadi · 2017-01-31T01:01:36Z

src/Workspaces/CoreTest/SolutionTests/SolutionTests.cs

+            try
+            {
+                // save temp file
+                tempPath = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName());


why not just Path.GetTempFileName.

Or even better: Temp.CreateDirectory()

And remove try-finally.

sure, will change to use that. I searched how other does this kind of thing in code base and did same thing.

this is added so that we can reduce chance of OOM when user added massive size text file to solution. current default max is 100MB. but user can override it through hidden reg key if they want to. if such file is added, FileTextLoader will throw InvalidDataException and Workspace will raise Workspace Failed event. in VS host case, we will show the failed event in output windows. made it to use test option service since option is mutable workspace state which affects all other tests.

heejaechang · 2017-01-31T08:58:03Z

ping?

CyrusNajmabadi · 2017-01-31T19:48:23Z

src/Workspaces/Core/Desktop/Workspace/FileTextLoader.cs

+        {
+            // Validate file length is under our threshold. 
+            // Otherwise, rather than reading the content into the memory, we will throw
+            // InvalidDataException to caller of FileTextLoader.LoadText to deal with 


can we add this exception to the documentation of the virtual calls?

CyrusNajmabadi · 2017-01-31T19:53:48Z

👍 Thanks for adding the better documentation!

natidea · 2017-01-31T22:32:05Z

Approved to merge via 377245

dnfclas added the cla-already-signed label Jan 26, 2017

heejaechang added PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. and removed cla-already-signed labels Jan 26, 2017

Pilchie reviewed Jan 26, 2017

View reviewed changes

jaredpar reviewed Jan 27, 2017

View reviewed changes

heejaechang force-pushed the filesizelimit branch 2 times, most recently from 6b21ae4 to 307bd8c Compare January 27, 2017 19:03

heejaechang removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jan 27, 2017

jaredpar approved these changes Jan 27, 2017

View reviewed changes

tmat reviewed Jan 27, 2017

View reviewed changes

heejaechang added the cla-already-signed label Jan 27, 2017

MattGertz added the Needs Shiproom Approval label Jan 27, 2017

jasonmalinowski approved these changes Jan 30, 2017

View reviewed changes

CyrusNajmabadi reviewed Jan 30, 2017

View reviewed changes

CyrusNajmabadi requested changes Jan 30, 2017

View reviewed changes

Pilchie added Area-IDE Tenet-Reliability Customer telemetry indicates that the product is failing in a crash/hang/dataloss manner. labels Jan 30, 2017

Pilchie added this to the 2.0 (RTM) milestone Jan 30, 2017

heejaechang force-pushed the filesizelimit branch from 20e29bd to 63f3a47 Compare January 30, 2017 23:02

CyrusNajmabadi reviewed Jan 31, 2017

View reviewed changes

heejaechang force-pushed the filesizelimit branch from e3e4b42 to e83cd71 Compare January 31, 2017 02:42

CyrusNajmabadi reviewed Jan 31, 2017

View reviewed changes

add xml doc on InvalidDataException

3c99db5

natidea added the Approved to merge label Jan 31, 2017

heejaechang merged commit 1e20fcc into dotnet:master Jan 31, 2017

Pilchie mentioned this pull request Feb 7, 2017

VS crashes when a massive file is part of a solution #16796

Closed

added max file limit #16775

added max file limit #16775

Conversation

heejaechang commented Jan 26, 2017 • edited Loading

Pilchie left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

heejaechang Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

heejaechang commented Jan 27, 2017

heejaechang commented Jan 27, 2017

Choose a reason for hiding this comment

heejaechang Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jac009 commented Jan 27, 2017

AlekseyTs commented Jan 27, 2017

heejaechang commented Jan 27, 2017 • edited Loading

tmat Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat commented Jan 27, 2017

heejaechang commented Jan 27, 2017

heejaechang commented Jan 27, 2017

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

jasonmalinowski commented Jan 30, 2017

CyrusNajmabadi commented Jan 30, 2017

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

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

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

heejaechang commented Jan 30, 2017

CyrusNajmabadi commented Jan 30, 2017

heejaechang commented Jan 30, 2017

heejaechang commented Jan 30, 2017

CyrusNajmabadi commented Jan 30, 2017

heejaechang commented Jan 30, 2017

MattGertz commented Jan 30, 2017

Pilchie commented Jan 30, 2017

CyrusNajmabadi commented Jan 30, 2017

heejaechang commented Jan 30, 2017

CyrusNajmabadi commented Jan 30, 2017

heejaechang commented Jan 30, 2017

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

heejaechang commented Jan 31, 2017

heejaechang commented Jan 26, 2017 •

edited

Loading

tmat Jan 27, 2017 •

edited

Loading

heejaechang Jan 27, 2017 •

edited

Loading

heejaechang Jan 27, 2017 •

edited

Loading

heejaechang commented Jan 27, 2017 •

edited

Loading

tmat Jan 27, 2017 •

edited

Loading