-
Notifications
You must be signed in to change notification settings - Fork 258
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
Comments
#4004 is a mono issue, Did you hit this on windows or mac? |
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. |
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. |
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. |
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.
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.
The text was updated successfully, but these errors were encountered: