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

Using SDK MethodInvoker rather than reflection for method invoke #6546

Merged
merged 1 commit into from Aug 24, 2020

Conversation

mathewc
Copy link
Member

@mathewc mathewc commented Aug 21, 2020

No description provided.


public ProxyEndToEndTests(TestFixture fixture)
{
_fixture = fixture;
_settingsManager = ScriptSettingsManager.Instance;
_hostName = new HostNameProvider(SystemEnvironment.Instance).Value ?? "localhost";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Often on my machine I have a WEBSITE_SITE_NAME env var set which breaks these tests.

@@ -267,7 +269,7 @@ protected override async Task<object> InvokeCore(object[] parameters, FunctionIn
int actualParameterCount = function.GetParameters().Length;
parameters = parameters.Take(actualParameterCount).ToArray();

object result = function.Invoke(null, parameters);
object result = await _methodInvoker.InvokeAsync(null, parameters);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replacing reflection invoke with call to compiled lambda. We're now using the same MethodInvokerFactory that the core SDK uses when invoking methods.

@@ -3,6 +3,7 @@
<packageSources>
<add key="nuget.org" value="https://www.nuget.org/api/v2/" />
<add key="azure_app_service" value="https://www.myget.org/F/azure-appservice/api/v2" />
<add key="azure-appservice-test" value="https://azfunc.pkgs.visualstudio.com/e6a70c92-4128-439f-8012-382fe78d6396/_packaging/azure-appservice-test%40Local/nuget/v3/index.json" />
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding new test package source I created in devops for intermediate test packages. E.g. I've uploaded packages that contain new SDK APIs this PR relies on, which allows the PR build to pass, before those changes are actually published.

@@ -64,7 +64,7 @@
<PackageReference Include="Microsoft.Azure.Services.AppAuthentication" Version="1.0.3" />
<PackageReference Include="Microsoft.Azure.Storage.File" Version="11.1.7" />
<PackageReference Include="Microsoft.Azure.WebJobs.Host.Storage" Version="4.0.1" />
<PackageReference Include="Microsoft.Azure.WebJobs" Version="3.0.20" />
<PackageReference Include="Microsoft.Azure.WebJobs" Version="3.0.21-dev637336297117359276" />
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Temp - will update with actual package version

// unwrap the task
if (result is Task)
{
result = await ((Task)result).ContinueWith(t => GetTaskResult(t), TaskContinuationOptions.ExecuteSynchronously);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer need to use reflection to unwrap the task result

brettsam
brettsam previously approved these changes Aug 24, 2020
@brettsam
Copy link
Member

I'll have to re-approve once you update the nugets.

@mathewc mathewc merged commit d22956e into dev Aug 24, 2020
@mathewc mathewc deleted the method-invoker branch August 24, 2020 21:40
@mathewc mathewc added the perf label Sep 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants