Guard against NullReference while creating HostContext #2343
Conversation
src/Runner.Common/HostContext.cs
Outdated
| @@ -221,8 +221,11 @@ public HostContext(string hostType, string logFile = null) | |||
| if (File.Exists(runnerFile)) | |||
| { | |||
| var runnerSettings = IOUtil.LoadObject<RunnerSettings>(runnerFile); | |||
| _userAgents.Add(new ProductInfoHeaderValue("RunnerId", runnerSettings.AgentId.ToString(CultureInfo.InvariantCulture))); | |||
| _userAgents.Add(new ProductInfoHeaderValue("GroupId", runnerSettings.PoolId.ToString(CultureInfo.InvariantCulture))); | |||
| if (runnerSettings != null) | |||
There was a problem hiding this comment.
should we just throw and crash the runner if runnerSettings is NULL?
There was a problem hiding this comment.
It is going to say that the runner is unauthenticated. This occurs when the file is empty, so json is invalid.
The intention is not to throw null reference here and leave it up to the client saying that the runner is not authenticated.
If you think it is better to provide a different message or throw other exception, I can work on that
There was a problem hiding this comment.
Maybe put the assert under IOUtil.LoadObject? ex: IOUtil.LoadObject(string file, bool required = false/true)
and the IOUtil.LoadObject can throw if it gets an empty file.
nikola-jokic
force-pushed
the
nikola-jokic/host-context-null-guard
branch
from
823a9f9 to
19a21de
Compare
src/Runner.Sdk/Util/IOUtil.cs
Outdated
| string json = File.ReadAllText(path, Encoding.UTF8); | ||
| if (string.IsNullOrEmpty(json)) | ||
| { | ||
| throw new InvalidOperationException($"File {path} is empty"); |
There was a problem hiding this comment.
@TingluoHuang any suggestions for exception that we should throw
There was a problem hiding this comment.
do we want to merge public static T LoadObject<T>(string path, bool required) merge with public static T LoadObject<T>(string path) to be something like public static T LoadObject<T>(string path, bool required = false)?
There was a problem hiding this comment.
ArgumentNullException or ArgumentException both seems fine to me.
There was a problem hiding this comment.
Applied ArgumentNullException if file is empty or null, ArgumentException if convert from json returns null
* Guard against NullReference while creating HostContext * Throw on IOUtil loading object if content is empty or object is null * removed unused dependency * Changed exceptions to ArgumentNullException and ArgumentException * fixed my mistake on LoadObject * fixed tests * trigger workflows * trigger workflows * encode files using utf8
* Guard against NullReference while creating HostContext * Throw on IOUtil loading object if content is empty or object is null * removed unused dependency * Changed exceptions to ArgumentNullException and ArgumentException * fixed my mistake on LoadObject * fixed tests * trigger workflows * trigger workflows * encode files using utf8
Fixes null reference exception creating
HostContextif the.runnerfile exists, but is empty.Null reference exception is described in issue #2342, but this does not solve the issue itself.
It just lets the HostContext proceed and the error message will be displayed as the runner is already configured.