You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@openwhisk.apache.org by "gleono (via GitHub)" <gi...@apache.org> on 2024/10/10 23:14:50 UTC
[PR] Enable Nullable Reference Types [openwhisk-runtime-dotnet]
gleono opened a new pull request, #92:
URL: https://github.com/apache/openwhisk-runtime-dotnet/pull/92
I mentioned that this could be a good improvement on the code for this runtime in #91
I probably got carried away with some other changes not specific to enabling NRTs. I will explain my rationale for these and ask questions about other minor details in the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enable Nullable Reference Types [openwhisk-runtime-dotnet]
Posted by "gleono (via GitHub)" <gi...@apache.org>.
gleono commented on code in PR #92:
URL: https://github.com/apache/openwhisk-runtime-dotnet/pull/92#discussion_r1796206678
##########
core/net6.0/proxy/Apache.OpenWhisk.Runtime.Common/HttpResponseExtension.cs:
##########
@@ -27,15 +27,14 @@ public static class HttpResponseExtension
{
public static async Task WriteResponse(this HttpResponse response, int code, string content)
{
- byte[] bytes = Encoding.UTF8.GetBytes(content);
- response.ContentLength = bytes.Length;
+ response.ContentLength = Encoding.UTF8.GetByteCount(content);
Review Comment:
No need to allocate an array to count the number of bytes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enable Nullable Reference Types [openwhisk-runtime-dotnet]
Posted by "gleono (via GitHub)" <gi...@apache.org>.
gleono commented on code in PR #92:
URL: https://github.com/apache/openwhisk-runtime-dotnet/pull/92#discussion_r1796213806
##########
core/net6.0/proxy/Apache.OpenWhisk.Runtime.Common/Init.cs:
##########
@@ -54,58 +53,52 @@ public async Task<Run> HandleRequest(HttpContext httpContext)
{
await httpContext.Response.WriteError("Cannot initialize the action more than once.");
Console.Error.WriteLine("Cannot initialize the action more than once.");
- return (new Run(Type, Method, Constructor, AwaitableMethod));
+ return new Run(_type, _method, _constructor, _awaitableMethod);
}
- string body = await new StreamReader(httpContext.Request.Body).ReadToEndAsync();
+ using StreamReader reader = new(httpContext.Request.Body);
+ string body = await reader.ReadToEndAsync();
JObject inputObject = JObject.Parse(body);
- if (!inputObject.ContainsKey("value"))
+ if (!inputObject.TryGetValue("value", out JToken? message) || message is not JObject valueObj)
Review Comment:
Use pattern matching to assign `valueObj` right away.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enable Nullable Reference Types [openwhisk-runtime-dotnet]
Posted by "gleono (via GitHub)" <gi...@apache.org>.
gleono commented on code in PR #92:
URL: https://github.com/apache/openwhisk-runtime-dotnet/pull/92#discussion_r1796210133
##########
core/net6.0/proxy/Apache.OpenWhisk.Runtime.Common/Init.cs:
##########
@@ -21,31 +21,30 @@
using System.Reflection;
using System.Threading;
using System.Threading.Tasks;
-using System.Collections.Generic;
using Microsoft.AspNetCore.Http;
using Newtonsoft.Json.Linq;
namespace Apache.OpenWhisk.Runtime.Common
{
public class Init
{
- private readonly SemaphoreSlim _initSemaphoreSlim = new SemaphoreSlim(1, 1);
+ private readonly SemaphoreSlim _initSemaphoreSlim = new(initialCount: 1, maxCount: 1);
+ private Type? _type;
+ private MethodInfo? _method;
+ private ConstructorInfo? _constructor;
+ private bool _awaitableMethod;
public bool Initialized { get; private set; }
- private Type Type { get; set; }
Review Comment:
private properties could very well be variables instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enable Nullable Reference Types [openwhisk-runtime-dotnet]
Posted by "gleono (via GitHub)" <gi...@apache.org>.
gleono commented on code in PR #92:
URL: https://github.com/apache/openwhisk-runtime-dotnet/pull/92#discussion_r1796216015
##########
core/net6.0/proxy/Apache.OpenWhisk.Runtime.Common/Init.cs:
##########
@@ -54,58 +53,52 @@ public async Task<Run> HandleRequest(HttpContext httpContext)
{
await httpContext.Response.WriteError("Cannot initialize the action more than once.");
Console.Error.WriteLine("Cannot initialize the action more than once.");
- return (new Run(Type, Method, Constructor, AwaitableMethod));
+ return new Run(_type, _method, _constructor, _awaitableMethod);
}
- string body = await new StreamReader(httpContext.Request.Body).ReadToEndAsync();
+ using StreamReader reader = new(httpContext.Request.Body);
+ string body = await reader.ReadToEndAsync();
JObject inputObject = JObject.Parse(body);
- if (!inputObject.ContainsKey("value"))
+ if (!inputObject.TryGetValue("value", out JToken? message) || message is not JObject valueObj)
{
await httpContext.Response.WriteError("Missing main/no code to execute.");
- return (null);
+ return null;
}
- JToken message = inputObject["value"];
+ string main = valueObj.TryGetValue("main", out JToken? mainToken) ? mainToken.ToString() : string.Empty;
+ string code = valueObj.TryGetValue("code", out JToken? codeToken) ? codeToken.ToString() : string.Empty;
+ bool binary = valueObj.TryGetValue("binary", out JToken? binaryToken) && binaryToken.ToObject<bool>();
- if (message["main"] == null || message["binary"] == null || message["code"] == null)
+ if (string.IsNullOrWhiteSpace(main) || string.IsNullOrWhiteSpace(code))
Review Comment:
These are checking only for white space because `main` and `code` are guaranteed to not be `null`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enable Nullable Reference Types [openwhisk-runtime-dotnet]
Posted by "gleono (via GitHub)" <gi...@apache.org>.
gleono commented on code in PR #92:
URL: https://github.com/apache/openwhisk-runtime-dotnet/pull/92#discussion_r1796220922
##########
core/net6.0/proxy/Apache.OpenWhisk.Runtime.Common/Run.cs:
##########
@@ -81,24 +82,24 @@ await Console.Error.WriteLineAsync(
}
}
- object owObject = _constructor.Invoke(new object[] { });
+ object owObject = _constructor.Invoke(Array.Empty<object>());
Review Comment:
No need to allocate an object to invoke the constructor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enable Nullable Reference Types [openwhisk-runtime-dotnet]
Posted by "gleono (via GitHub)" <gi...@apache.org>.
gleono commented on code in PR #92:
URL: https://github.com/apache/openwhisk-runtime-dotnet/pull/92#discussion_r1796220922
##########
core/net6.0/proxy/Apache.OpenWhisk.Runtime.Common/Run.cs:
##########
@@ -81,24 +82,24 @@ await Console.Error.WriteLineAsync(
}
}
- object owObject = _constructor.Invoke(new object[] { });
+ object owObject = _constructor.Invoke(Array.Empty<object>());
Review Comment:
No need to allocate an object to make invoke the constructor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enable Nullable Reference Types [openwhisk-runtime-dotnet]
Posted by "gleono (via GitHub)" <gi...@apache.org>.
gleono commented on code in PR #92:
URL: https://github.com/apache/openwhisk-runtime-dotnet/pull/92#discussion_r1796215000
##########
core/net6.0/proxy/Apache.OpenWhisk.Runtime.Common/Init.cs:
##########
@@ -54,58 +53,52 @@ public async Task<Run> HandleRequest(HttpContext httpContext)
{
await httpContext.Response.WriteError("Cannot initialize the action more than once.");
Console.Error.WriteLine("Cannot initialize the action more than once.");
- return (new Run(Type, Method, Constructor, AwaitableMethod));
+ return new Run(_type, _method, _constructor, _awaitableMethod);
}
- string body = await new StreamReader(httpContext.Request.Body).ReadToEndAsync();
+ using StreamReader reader = new(httpContext.Request.Body);
+ string body = await reader.ReadToEndAsync();
JObject inputObject = JObject.Parse(body);
- if (!inputObject.ContainsKey("value"))
+ if (!inputObject.TryGetValue("value", out JToken? message) || message is not JObject valueObj)
{
await httpContext.Response.WriteError("Missing main/no code to execute.");
- return (null);
+ return null;
Review Comment:
I am not sure why all return values are between parentheses. Is this enforced by the Scalariform plugin?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enable Nullable Reference Types [openwhisk-runtime-dotnet]
Posted by "gleono (via GitHub)" <gi...@apache.org>.
gleono commented on code in PR #92:
URL: https://github.com/apache/openwhisk-runtime-dotnet/pull/92#discussion_r1796210962
##########
core/net6.0/proxy/Apache.OpenWhisk.Runtime.Common/Init.cs:
##########
@@ -21,31 +21,30 @@
using System.Reflection;
using System.Threading;
using System.Threading.Tasks;
-using System.Collections.Generic;
using Microsoft.AspNetCore.Http;
using Newtonsoft.Json.Linq;
namespace Apache.OpenWhisk.Runtime.Common
{
public class Init
{
- private readonly SemaphoreSlim _initSemaphoreSlim = new SemaphoreSlim(1, 1);
+ private readonly SemaphoreSlim _initSemaphoreSlim = new(initialCount: 1, maxCount: 1);
Review Comment:
Shorter syntax and clearer parameters.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enable Nullable Reference Types [openwhisk-runtime-dotnet]
Posted by "gleono (via GitHub)" <gi...@apache.org>.
gleono commented on code in PR #92:
URL: https://github.com/apache/openwhisk-runtime-dotnet/pull/92#discussion_r1796216613
##########
core/net6.0/proxy/Apache.OpenWhisk.Runtime.Common/Init.cs:
##########
@@ -54,58 +53,52 @@ public async Task<Run> HandleRequest(HttpContext httpContext)
{
await httpContext.Response.WriteError("Cannot initialize the action more than once.");
Console.Error.WriteLine("Cannot initialize the action more than once.");
- return (new Run(Type, Method, Constructor, AwaitableMethod));
+ return new Run(_type, _method, _constructor, _awaitableMethod);
}
- string body = await new StreamReader(httpContext.Request.Body).ReadToEndAsync();
+ using StreamReader reader = new(httpContext.Request.Body);
+ string body = await reader.ReadToEndAsync();
JObject inputObject = JObject.Parse(body);
- if (!inputObject.ContainsKey("value"))
+ if (!inputObject.TryGetValue("value", out JToken? message) || message is not JObject valueObj)
{
await httpContext.Response.WriteError("Missing main/no code to execute.");
- return (null);
+ return null;
}
- JToken message = inputObject["value"];
+ string main = valueObj.TryGetValue("main", out JToken? mainToken) ? mainToken.ToString() : string.Empty;
+ string code = valueObj.TryGetValue("code", out JToken? codeToken) ? codeToken.ToString() : string.Empty;
+ bool binary = valueObj.TryGetValue("binary", out JToken? binaryToken) && binaryToken.ToObject<bool>();
- if (message["main"] == null || message["binary"] == null || message["code"] == null)
+ if (string.IsNullOrWhiteSpace(main) || string.IsNullOrWhiteSpace(code))
{
await httpContext.Response.WriteError("Missing main/no code to execute.");
- return (null);
+ return null;
}
- string main = message["main"].ToString();
-
- bool binary = message["binary"].ToObject<bool>();
-
if (!binary)
{
await httpContext.Response.WriteError("code must be binary (zip file).");
- return (null);
+ return null;
}
string[] mainParts = main.Split("::");
if (mainParts.Length != 3)
{
await httpContext.Response.WriteError("main required format is \"Assembly::Type::Function\".");
- return (null);
+ return null;
}
string tempPath = Path.Combine(Environment.CurrentDirectory, Guid.NewGuid().ToString());
- string base64Zip = message["code"].ToString();
try
{
- using (MemoryStream stream = new MemoryStream(Convert.FromBase64String(base64Zip)))
- {
- using (ZipArchive archive = new ZipArchive(stream))
- {
- archive.ExtractToDirectory(tempPath);
- }
- }
+ using MemoryStream stream = new(Convert.FromBase64String(code));
Review Comment:
No need to nest `using`s anymore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enable Nullable Reference Types [openwhisk-runtime-dotnet]
Posted by "gleono (via GitHub)" <gi...@apache.org>.
gleono commented on code in PR #92:
URL: https://github.com/apache/openwhisk-runtime-dotnet/pull/92#discussion_r1796220713
##########
core/net6.0/proxy/Apache.OpenWhisk.Runtime.Common/Run.cs:
##########
@@ -49,12 +49,13 @@ public async Task HandleRequest(HttpContext httpContext)
try
{
- string body = await new StreamReader(httpContext.Request.Body).ReadToEndAsync();
+ using StreamReader reader = new(httpContext.Request.Body);
Review Comment:
Let's be good citizens and dispose any unmanaged resources created by the `StreamReader`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enable Nullable Reference Types [openwhisk-runtime-dotnet]
Posted by "gleono (via GitHub)" <gi...@apache.org>.
gleono commented on code in PR #92:
URL: https://github.com/apache/openwhisk-runtime-dotnet/pull/92#discussion_r1796209273
##########
core/net6.0/proxy/Apache.OpenWhisk.Runtime.Common/HttpResponseExtension.cs:
##########
@@ -27,15 +27,14 @@ public static class HttpResponseExtension
{
public static async Task WriteResponse(this HttpResponse response, int code, string content)
{
- byte[] bytes = Encoding.UTF8.GetBytes(content);
- response.ContentLength = bytes.Length;
+ response.ContentLength = Encoding.UTF8.GetByteCount(content);
response.StatusCode = code;
await response.WriteAsync(content);
}
public static async Task WriteError(this HttpResponse response, string errorMessage)
{
- JObject message = new JObject {{"error", new JValue(errorMessage)}};
+ JObject message = new() {{"error", new JValue(errorMessage)}};
Review Comment:
Shorter syntax for constructors. Type is already in the declaration of the variable so, no need to be redundant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enable Nullable Reference Types [openwhisk-runtime-dotnet]
Posted by "gleono (via GitHub)" <gi...@apache.org>.
gleono commented on code in PR #92:
URL: https://github.com/apache/openwhisk-runtime-dotnet/pull/92#discussion_r1796220713
##########
core/net6.0/proxy/Apache.OpenWhisk.Runtime.Common/Run.cs:
##########
@@ -49,12 +49,13 @@ public async Task HandleRequest(HttpContext httpContext)
try
{
- string body = await new StreamReader(httpContext.Request.Body).ReadToEndAsync();
+ using StreamReader reader = new(httpContext.Request.Body);
Review Comment:
Let's be good citizens and dispose any managed resources created by the `StreamReader`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enable Nullable Reference Types [openwhisk-runtime-dotnet]
Posted by "gleono (via GitHub)" <gi...@apache.org>.
gleono commented on PR #92:
URL: https://github.com/apache/openwhisk-runtime-dotnet/pull/92#issuecomment-2406237215
I forgot to mention that I still cannot run the tests locally in my machine since I can only run openwhisk in standalone mode in a docker container and I don't know how to run the tests against it.
I tested manually by running the container for this runtime and sending the base64 encoded tests to it in the expected JSON text. It seems to work fine that way.
I found some issues while trying to test it in integration with openwhisk in standalone mode. Specially the array input and output cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org