Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nuget fails with TFS credential provider #6572

Open
TheJayMann opened this issue Feb 15, 2018 · 4 comments
Open

Nuget fails with TFS credential provider #6572

TheJayMann opened this issue Feb 15, 2018 · 4 comments
Labels
Area:Authentication Priority:3 Issues under consideration. With enough upvotes, will be reconsidered to be added to the backlog. Status:Excluded from icebox cleanup Status:Inactive Icebox issues not updated for a specific long time
Milestone

Comments

@TheJayMann
Copy link

In about 90%+ of the cases when using the nuget task in a TFS 2017 release definition, using version 4.0.0, while also allowing access to the OAuth token, nuget failes with an InvalidOperationException, stating that the process has exited. Following the stack trace, I found that it occurs in the PluginCredentialProvider class in an anonymous delegate defined in the Execute method. A call to process.ProcessName is causing the exception. I see that a workaround was added for #4004, such that this property is only accessed if the process has not yet exited. Given this case, that means that either process.HasExited or process.ProcessName is lying as to whether the process has already exited, or the process happens to exit between the call to process.HasExited and the call to process.ProcessName.

The reason I raise this as an issue is that this condition happens so often that when it does work properly appears to be the exception rather than the rule.

@zhili1208 zhili1208 added this to the Backlog milestone Feb 15, 2018
@zhili1208
Copy link
Contributor

#4004 is a mono issue, Did you hit this on windows or mac?

@TheJayMann
Copy link
Author

TheJayMann commented Feb 15, 2018

Never said this was the same as #4004, rather that the same problem was occurring, even though a workaround was put in place. I was actually able to resolve this problem, but the code changes are still on my work PC.

This is on Windows and does not involve mono.

@zhili1208 zhili1208 added the Priority:2 Issues for the current backlog. label Feb 15, 2018
@TheJayMann
Copy link
Author

This is a quick patch which appears to have fixed the issue for me (I was able to run the build and release twice in a row with success for the first time).

diff --git a/src/NuGet.Clients/NuGet.Credentials/PluginCredentialProvider.cs b/src/NuGet.Clients/NuGet.Credentials/Plugi
nCredentialProvider.cs
index e3cab4806..040391f89 100644
--- a/src/NuGet.Clients/NuGet.Credentials/PluginCredentialProvider.cs
+++ b/src/NuGet.Clients/NuGet.Credentials/PluginCredentialProvider.cs
@@ -248,7 +248,16 @@ public virtual int Execute(ProcessStartInfo startInfo, CancellationToken cancell
             {
                 throw PluginException.CreateNotStartedMessage(Path);
             }
-
+
+            string processName;
+            try
+            {
+                processName = process.ProcessName;
+            }
+            catch (InvalidOperationException)
+            {
+                processName = System.IO.Path.GetFileName(startInfo.FileName);
+            }
             process.OutputDataReceived += (object o, DataReceivedEventArgs e) => { outBuffer.AppendLine(e.Data); };

            // Trace and error information may be written to standard error by the provider.
            // It should be logged at the Information level so it will appear if Verbosity >= Normal.
            process.ErrorDataReceived += (object o, DataReceivedEventArgs e) =>
            {
                if (!string.IsNullOrWhiteSpace(e?.Data))
                 {
-                    // This is a workaround for mono issue: https://github.com/NuGet/Home/issues/4004
-                    if (!process.HasExited)
-                    {
-                        _logger.LogInformation($"{process.ProcessName}: {e.Data}");
-                    }
+                    _logger.LogInformation($"{processName}: {e.Data}");
                 }
             };

Best I can say is that, for some reason, on my setup, the credential provider sometimes completes so quickly that any attempt to get the ProcessName property throws an InvalidOperationException due to the process already having exited. Either that, or the ProcessName property for some reason believes the process has already exited when it hasn't.

@CosminRadu
Copy link

CosminRadu commented Sep 22, 2018

I've been hitting the same issue on some of our build servers. @TheJayMann, I hope you don't mind, but I went ahead and created PR 2433 with your suggested fix.

nkolev92 pushed a commit to CosminRadu/NuGet.Client that referenced this issue May 24, 2019
Details:
The code in PluginCredentailProvider.Execute() suffers from a race condition between the call to Process.HasExited returning false and the following call to Process.ProcessName. The fix has been proposed by TheJayMann (https://github.com/TheJayMann) in his issue report: NuGet/Home#6572.
@ghost ghost added the Status:Inactive Icebox issues not updated for a specific long time label Sep 1, 2022
@nkolev92 nkolev92 added Priority:3 Issues under consideration. With enough upvotes, will be reconsidered to be added to the backlog. and removed Priority:2 Issues for the current backlog. labels Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area:Authentication Priority:3 Issues under consideration. With enough upvotes, will be reconsidered to be added to the backlog. Status:Excluded from icebox cleanup Status:Inactive Icebox issues not updated for a specific long time
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants