Analyzer / generator shadow load directory needs to be per user on Unix #65415
Comments
dotnet-issue-labeler
bot
added
the
untriaged
jaredpar
removed
the
untriaged
|
Looks related to #61900 |
|
I also think I saw either the sdk or nuget running into a similar issue but can't find the relevant GitHub issues. |
Curiously that issue is asking for permissions to be made more restrictive (755->700), whereas the "quick fix" for this issue would be be less restrictive (755->777). (Agree that the proper fix for this would be to create directories per user). |
|
I just found the change I had in mind: dotnet/msbuild@7f3a30c |
|
Is there a chance this would get fixed sooner than 17.6? |
jaredpar
pushed a commit
to jaredpar/roslyn
that referenced
this issue
May 18, 2023
Before fixing dotnet#65415 I needed to better rationalize how `Path.GetTempPath` and temp paths in general worked in the compiler. This change ended up addressing the following problems: `Path.GetTempPath` should never be used in the compiler libraries. That uses a number of environment variables, like working directory, that can change during the lifetime of a build operation. That makes compilation non-deterministic and can lead to sporadic errors. All uses of `GetTempPath` need to come from the initial csc / vbc invocation and be passed through as arguments. This was solved by adding a new BannedSymbols file for compiler libraries that banned `Path.GetTempPath` and working through the remaining case I found. This introduced one new error because `Path.GetTempPath` was being used as a fall back in legacy signing if csc / vbc couldn't calculate a temp path at invocation time. This is a "this should never actually happen" problem but I added an error code as a precaution. I did not force the temp directory to non-null for all compilations because the vast majority of compilations don't actually need a temp directory. It is only used in legacy signing which is very rare. It was unclear in our code base where a temp directory was actually required for execution. To get a better handle on this, now and going forward, I enabled NRT in files that handled temp directories. After working through it there are only two cases where it's required: 1. When using legacy strong named signing. 2. When starting the compiler server. For (2) it's required as a place to shadow copy analyzers / generators. There is no reasonable fall back if the server can't calculate a temp path. Further if the client can't calculate a temp path then there is a very strong chance (basically guaranteed) that the server can't either. Therefore I added a few places where we simply don't even try to communicate with the compiler server if we can't find a temporary directory. Basically fail fast vs. failing slow. Once this change is in I can move forward with dotnet#65415. The impact of that change will be much clearer after this goes through because it changes both what we depend on and where shadow copy happens.
jaredpar
pushed a commit
to jaredpar/roslyn
that referenced
this issue
May 18, 2023
Before fixing dotnet#65415 I needed to better rationalize how `Path.GetTempPath` and temp paths in general worked in the compiler. This change ended up addressing the following problems: **Path.GetTempPath** `Path.GetTempPath` should never be used in the compiler libraries. That uses a number of environment variables, like working directory, that can change during the lifetime of a build operation. That makes compilation non-deterministic and can lead to sporadic errors. All uses of `GetTempPath` need to come from the initial csc / vbc invocation and be passed through as arguments. This was solved by adding a new BannedSymbols file for compiler libraries that banned `Path.GetTempPath` and working through the remaining case I found. This introduced one new error because `Path.GetTempPath` was being used as a fall back in legacy signing if csc / vbc couldn't calculate a temp path at invocation time. This is a "this should never actually happen" problem but I added an error code as a precaution. I did not force the temp directory to non-null for all compilations because the vast majority of compilations don't actually need a temp directory. It is only used in legacy signing which is very rare. **Where is temp required** It was unclear in our code base where a temp directory was actually required for execution. To get a better handle on this, now and going forward, I enabled NRT in files that handled temp directories. After working through it there are only two cases where it's required: 1. When using legacy strong named signing. 2. When starting the compiler server. For (2) it's required as a place to shadow copy analyzers / generators. There is no reasonable fall back if the server can't calculate a temp path. Further if the client can't calculate a temp path then there is a very strong chance (basically guaranteed) that the server can't either. Therefore I added a few places where we simply don't even try to communicate with the compiler server if we can't find a temporary directory. Basically fail fast vs. failing slow. Once this change is in I can move forward with dotnet#65415. The impact of that change will be much clearer after this goes through because it changes both what we depend on and where shadow copy happens.
jaredpar
pushed a commit
to jaredpar/roslyn
that referenced
this issue
Jul 31, 2023
Before fixing dotnet#65415 I needed to better rationalize how `Path.GetTempPath` and temp paths in general worked in the compiler. This change ended up addressing the following problems: **Path.GetTempPath** `Path.GetTempPath` should never be used in the compiler libraries. That uses a number of environment variables, like working directory, that can change during the lifetime of a build operation. That makes compilation non-deterministic and can lead to sporadic errors. All uses of `GetTempPath` need to come from the initial csc / vbc invocation and be passed through as arguments. This was solved by adding a new BannedSymbols file for compiler libraries that banned `Path.GetTempPath` and working through the remaining case I found. This introduced one new error because `Path.GetTempPath` was being used as a fall back in legacy signing if csc / vbc couldn't calculate a temp path at invocation time. This is a "this should never actually happen" problem but I added an error code as a precaution. I did not force the temp directory to non-null for all compilations because the vast majority of compilations don't actually need a temp directory. It is only used in legacy signing which is very rare. **Where is temp required** It was unclear in our code base where a temp directory was actually required for execution. To get a better handle on this, now and going forward, I enabled NRT in files that handled temp directories. After working through it there are only two cases where it's required: 1. When using legacy strong named signing. 2. When starting the compiler server. For (2) it's required as a place to shadow copy analyzers / generators. There is no reasonable fall back if the server can't calculate a temp path. Further if the client can't calculate a temp path then there is a very strong chance (basically guaranteed) that the server can't either. Therefore I added a few places where we simply don't even try to communicate with the compiler server if we can't find a temporary directory. Basically fail fast vs. failing slow. Once this change is in I can move forward with dotnet#65415. The impact of that change will be much clearer after this goes through because it changes both what we depend on and where shadow copy happens.
jaredpar
pushed a commit
to jaredpar/roslyn
that referenced
this issue
Aug 3, 2023
Before fixing dotnet#65415 I needed to better rationalize how `Path.GetTempPath` and temp paths in general worked in the compiler. This change ended up addressing the following problems: **Path.GetTempPath** `Path.GetTempPath` should never be used in the compiler libraries. That uses a number of environment variables, like working directory, that can change during the lifetime of a build operation. That makes compilation non-deterministic and can lead to sporadic errors. All uses of `GetTempPath` need to come from the initial csc / vbc invocation and be passed through as arguments. This was solved by adding a new BannedSymbols file for compiler libraries that banned `Path.GetTempPath` and working through the remaining case I found. This introduced one new error because `Path.GetTempPath` was being used as a fall back in legacy signing if csc / vbc couldn't calculate a temp path at invocation time. This is a "this should never actually happen" problem but I added an error code as a precaution. I did not force the temp directory to non-null for all compilations because the vast majority of compilations don't actually need a temp directory. It is only used in legacy signing which is very rare. **Where is temp required** It was unclear in our code base where a temp directory was actually required for execution. To get a better handle on this, now and going forward, I enabled NRT in files that handled temp directories. After working through it there are only two cases where it's required: 1. When using legacy strong named signing. 2. When starting the compiler server. For (2) it's required as a place to shadow copy analyzers / generators. There is no reasonable fall back if the server can't calculate a temp path. Further if the client can't calculate a temp path then there is a very strong chance (basically guaranteed) that the server can't either. Therefore I added a few places where we simply don't even try to communicate with the compiler server if we can't find a temporary directory. Basically fail fast vs. failing slow. Once this change is in I can move forward with dotnet#65415. The impact of that change will be much clearer after this goes through because it changes both what we depend on and where shadow copy happens.
jaredpar
added a commit
that referenced
this issue
Aug 3, 2023
* Rationalize how temporary directory works today Before fixing #65415 I needed to better rationalize how `Path.GetTempPath` and temp paths in general worked in the compiler. This change ended up addressing the following problems: **Path.GetTempPath** `Path.GetTempPath` should never be used in the compiler libraries. That uses a number of environment variables, like working directory, that can change during the lifetime of a build operation. That makes compilation non-deterministic and can lead to sporadic errors. All uses of `GetTempPath` need to come from the initial csc / vbc invocation and be passed through as arguments. This was solved by adding a new BannedSymbols file for compiler libraries that banned `Path.GetTempPath` and working through the remaining case I found. This introduced one new error because `Path.GetTempPath` was being used as a fall back in legacy signing if csc / vbc couldn't calculate a temp path at invocation time. This is a "this should never actually happen" problem but I added an error code as a precaution. I did not force the temp directory to non-null for all compilations because the vast majority of compilations don't actually need a temp directory. It is only used in legacy signing which is very rare. **Where is temp required** It was unclear in our code base where a temp directory was actually required for execution. To get a better handle on this, now and going forward, I enabled NRT in files that handled temp directories. After working through it there are only two cases where it's required: 1. When using legacy strong named signing. 2. When starting the compiler server. For (2) it's required as a place to shadow copy analyzers / generators. There is no reasonable fall back if the server can't calculate a temp path. Further if the client can't calculate a temp path then there is a very strong chance (basically guaranteed) that the server can't either. Therefore I added a few places where we simply don't even try to communicate with the compiler server if we can't find a temporary directory. Basically fail fast vs. failing slow. Once this change is in I can move forward with #65415. The impact of that change will be much clearer after this goes through because it changes both what we depend on and where shadow copy happens. * Change * PR feedback * undelete settings.json * Clarify stream relationship * PR feedback --------- Co-authored-by: Jared Parsons <[email protected]>
jaredpar
pushed a commit
to jaredpar/roslyn
that referenced
this issue
Aug 4, 2023
This changes analyzer loading on Linux to be per user where before clashes could occur in the temp directory. This fix initially started out by making our temp directory logic more robust. Essentially add per user sub directories, do extended permission checks, etc ... That exacerbated a number of existing corner cases in the code and forced us to put a lot more error handling into the workspaces layer. After discussion with the IDE team I decided to take the approach of loading from a `Stream` on non-Windows platforms. The performance difference is neglible (1ms to 2ms per assembly) and it removes the overhead of copying them around on disk as well as the complexities that come with managing the shadow copy directory. This will be observable to any analyzer that used `Assembly.Location` as it will now be `""`. That is only useful though for inspecting disks which is already illegal for analyzers hence it's considered an acceptable break. closes dotnet#65415
jaredpar
added a commit
that referenced
this issue
Aug 9, 2023
This changes analyzer loading on Linux to be per user where before clashes could occur in the temp directory. This fix initially started out by making our temp directory logic more robust. Essentially add per user sub directories, do extended permission checks, etc ... That exacerbated a number of existing corner cases in the code and forced us to put a lot more error handling into the workspaces layer. After discussion with the IDE team I decided to take the approach of loading from a `Stream` on non-Windows platforms. The performance difference is neglible (1ms to 2ms per assembly) and it removes the overhead of copying them around on disk as well as the complexities that come with managing the shadow copy directory. This will be observable to any analyzer that used `Assembly.Location` as it will now be `""`. That is only useful though for inspecting disks which is already illegal for analyzers hence it's considered an acceptable break. To test the performance of this change I wrote a simple benchmark that loads assemblies directly from path, copy then load (simulate shadow copy) and loading them from stream. The results on Windows is pretty striking |Method | Mean | Error | StdDev | Median| -- | -- | -- | -- | --| |LoadViaPath | 1.552 ms | 0.0387 ms | 0.1142 ms | 1.529 ms| |LoadViaPathShadow | 33.147 ms | 0.9000 ms | 2.6537 ms | 33.883 ms| |LoadViaStream | 495.945 ms | 9.5040 ms | 9.7599 ms | 493.297 ms| Notice the penalty of `LoadFromStream` is severe on Windows. That is due to the AV having to run on every load since it can't cache like it can when loading from path. On Linux though the results are much nicer |Method | Mean | Error | StdDev | Median| |-- | -- | -- | -- | --| |LoadViaPath | 1.301 ms | 0.0258 ms | 0.0326 ms | 1.299 ms| |LoadViaPathShadow | 4.126 ms | 0.3567 ms | 1.0060 ms | 3.742 ms| |LoadViaStream | 2.514 ms | 0.0501 ms | 0.0536 ms | 2.497 m| This means the change should be a small win on Linux but probably not in the noticable range. closes #65415
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Context: dotnet/runtime#77723
The root cause of this problem is the temp directory on Unix is the global one while on Windows it's per user. Hence there is a bit of a disconnect on how the shadow copy logic works there. Need to add a user component to the path on Unix to make things consistent here.
https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html
The text was updated successfully, but these errors were encountered: