Fixing Node memory leak (#2313) #2320
Conversation
|
|
||
| var bindings = new Dictionary<string, object>(); | ||
| var bind = (ScriptFunc)(p => |
There was a problem hiding this comment.
This bind function goes away. In working on this fix, I was trying to minimize the bi-directional calls between .NET and Node (which is the source of the memory leaks). In this case, rather than having Node call back into .NET to process binding results, I just return them as part of a structured function result, and allow them to be processed after control returns back to .NET.
| @@ -348,70 +351,14 @@ protected override void Dispose(bool disposing) | |||
|
|
|||
| private async Task<Dictionary<string, object>> CreateScriptExecutionContextAsync(object input, DataType dataType, TraceWriter traceWriter, FunctionInvocationContext invocationContext) | |||
| { | |||
| // create a TraceWriter wrapper that can be exposed to Node.js | |||
| var log = (ScriptFunc)(p => | |||
There was a problem hiding this comment.
The log and metric functions I pushed out into static factory methods, to prevent regressions. The factory functions take WeakReferences, so we won't accidentally regress things going forward.
| } | ||
| context.bind(values, function (err) { |
There was a problem hiding this comment.
Previously we were calling back into .NET here to allow it to process these binding values. We were doing this to enable us to capture any context.bindings[xxx] values the user code might have set. Edge.JS interop won't capture new properties added to objects/dictionaries.
With my changes, rather than call back into .NET to copy these values into the bindings collection, we just return the values as part of the response.
| } | ||
| } | ||
|
|
||
| return Task.FromResult<object>(null); |
There was a problem hiding this comment.
That doesn't work because this is a Task<object> not Task
There was a problem hiding this comment.
👍
Just looked at this line (not the signature) and assumed this was just using an old pattern from the existing code.
Fix for #2313.