Skip to content

Commit e991260

Browse files
mathewcmathewc
committed
Fixing memory leak (#2313)
1 parent f6f5fb3 commit e991260

File tree

2 files changed

+98
-79
lines changed

2 files changed

+98
-79
lines changed

‎src/WebJobs.Script/Description/Node/NodeFunctionInvoker.cs‎

Lines changed: 85 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -243,24 +243,27 @@ private static async Task ProcessOutputBindingsAsync(Collection<FunctionBinding>
243243
return;
244244
}
245245

246-
var bindings = (Dictionary<string, object>)scriptExecutionContext["bindings"];
246+
var functionResultDictionary = (IDictionary<string, object>)functionResult;
247+
var returnValue = functionResultDictionary["returnValue"];
248+
var bindingValues = (IDictionary<string, object>)functionResultDictionary["bindingValues"];
249+
247250
var returnValueBinding = outputBindings.SingleOrDefault(p => p.Metadata.IsReturn);
248251
if (returnValueBinding != null)
249252
{
250253
// if there is a $return binding, bind the entire function return to it
251254
// if additional output bindings need to be bound, they'll have to use the explicit
252255
// context.bindings mechanism to set values, not return value.
253-
bindings[ScriptConstants.SystemReturnParameterBindingName] = functionResult;
256+
bindingValues[ScriptConstants.SystemReturnParameterBindingName] = returnValue;
254257
}
255258
else
256259
{
257260
// if the function returned binding values via the function result,
258261
// apply them to context.bindings
259-
if (functionResult is IDictionary<string, object> functionOutputs)
262+
if (returnValue is IDictionary<string, object> functionOutputs)
260263
{
261264
foreach (var output in functionOutputs)
262265
{
263-
bindings[output.Key] = output.Value;
266+
bindingValues[output.Key] = output.Value;
264267
}
265268
}
266269
}
@@ -269,13 +272,13 @@ private static async Task ProcessOutputBindingsAsync(Collection<FunctionBinding>
269272
{
270273
// get the output value from the script
271274
object value = null;
272-
bool haveValue = bindings.TryGetValue(binding.Metadata.Name, out value);
275+
bool haveValue = bindingValues.TryGetValue(binding.Metadata.Name, out value);
273276
if (!haveValue && string.Compare(binding.Metadata.Type, "http", StringComparison.OrdinalIgnoreCase) == 0)
274277
{
275278
// http bindings support a special context.req/context.res programming
276279
// model, so we must map that back to the actual binding name if a value
277280
// wasn't provided using the binding name itself
278-
haveValue = bindings.TryGetValue("res", out value);
281+
haveValue = bindingValues.TryGetValue("res", out value);
279282
}
280283

281284
// apply the value to the binding
@@ -348,70 +351,14 @@ protected override void Dispose(bool disposing)
348351

349352
private async Task<Dictionary<string, object>> CreateScriptExecutionContextAsync(object input, DataType dataType, TraceWriter traceWriter, FunctionInvocationContext invocationContext)
350353
{
351-
// create a TraceWriter wrapper that can be exposed to Node.js
352-
var log = (ScriptFunc)(p =>
353-
{
354-
var logData = (IDictionary<string, object>)p;
355-
string message = (string)logData["msg"];
356-
if (message != null)
357-
{
358-
try
359-
{
360-
TraceLevel level = (TraceLevel)logData["lvl"];
361-
var evt = new TraceEvent(level, message);
362-
363-
// Node captures the AsyncLocal value of the first invocation, which means that logs
364-
// are correlated incorrectly. Here we'll overwrite that value with the correct value
365-
// immediately before logging.
366-
using (BeginFunctionScope(invocationContext))
367-
{
368-
// TraceWriter already logs to ILogger
369-
traceWriter.Trace(evt);
370-
}
371-
}
372-
catch (ObjectDisposedException)
373-
{
374-
// if a function attempts to write to a disposed
375-
// TraceWriter. Might happen if a function tries to
376-
// log after calling done()
377-
}
378-
}
379-
380-
return Task.FromResult<object>(null);
381-
});
382-
383-
var metric = (ScriptFunc)(p =>
384-
{
385-
var metricObject = (IDictionary<string, object>)p;
386-
string name = (string)metricObject["name"];
387-
object valueObject = metricObject["value"];
388-
389-
if (name != null && valueObject != null)
390-
{
391-
double value = Convert.ToDouble(valueObject);
392-
393-
// properties are optional
394-
var properties = (IDictionary<string, object>)metricObject["properties"];
395-
396-
using (BeginFunctionScope(invocationContext))
397-
{
398-
invocationContext.Logger.LogMetric(name, value, properties);
399-
}
400-
}
401-
402-
return Task.FromResult<object>(null);
403-
});
404-
405-
var bindings = new Dictionary<string, object>();
406-
var bind = (ScriptFunc)(p =>
407-
{
408-
IDictionary<string, object> bindValues = (IDictionary<string, object>)p;
409-
foreach (var bindValue in bindValues)
410-
{
411-
bindings[bindValue.Key] = bindValue.Value;
412-
}
413-
return Task.FromResult<object>(null);
414-
});
354+
// It's important that we use WeakReferences here for objects captured by
355+
// these function closures. Otherwise, due to the nature of how we're using Edge.js
356+
// we'll get memory leaks (we're establishing bi-directional communication between
357+
// .NET and Node).
358+
var traceWriterReference = new WeakReference<TraceWriter>(traceWriter);
359+
var invocationContextReference = new WeakReference<FunctionInvocationContext>(invocationContext);
360+
var log = CreateLoggerFunc(traceWriterReference, invocationContextReference);
361+
var metric = CreateMetricFunc(invocationContextReference);
415362

416363
var executionContext = new Dictionary<string, object>
417364
{
@@ -420,14 +367,14 @@ private async Task<Dictionary<string, object>> CreateScriptExecutionContextAsync
420367
["functionDirectory"] = invocationContext.ExecutionContext.FunctionDirectory,
421368
};
422369

370+
var bindings = new Dictionary<string, object>();
423371
var context = new Dictionary<string, object>()
424372
{
425373
{ "invocationId", invocationContext.ExecutionContext.InvocationId },
426374
{ "executionContext", executionContext },
427-
{ "log", log },
428-
{ "_metric", metric },
429375
{ "bindings", bindings },
430-
{ "bind", bind }
376+
{ "log", log },
377+
{ "_metric", metric }
431378
};
432379

433380
if (!string.IsNullOrEmpty(_entryPoint))
@@ -495,6 +442,71 @@ private async Task<Dictionary<string, object>> CreateScriptExecutionContextAsync
495442
return context;
496443
}
497444

445+
private static ScriptFunc CreateLoggerFunc(WeakReference<TraceWriter> traceWriterReference, WeakReference<FunctionInvocationContext> invocationContextReference)
446+
{
447+
return (ScriptFunc)(p =>
448+
{
449+
var logData = (IDictionary<string, object>)p;
450+
string message = (string)logData["msg"];
451+
if (message != null)
452+
{
453+
try
454+
{
455+
// Node captures the AsyncLocal value of the first invocation, which means that logs
456+
// are correlated incorrectly. Here we'll overwrite that value with the correct value
457+
// immediately before logging.
458+
TraceWriter traceWriter = null;
459+
FunctionInvocationContext invocationContext = null;
460+
if (traceWriterReference.TryGetTarget(out traceWriter) &&
461+
invocationContextReference.TryGetTarget(out invocationContext))
462+
{
463+
using (BeginFunctionScope(invocationContext))
464+
{
465+
// TraceWriter already logs to ILogger
466+
TraceLevel level = (TraceLevel)logData["lvl"];
467+
var evt = new TraceEvent(level, message);
468+
traceWriter.Trace(evt);
469+
}
470+
}
471+
}
472+
catch (ObjectDisposedException)
473+
{
474+
// if a function attempts to write to a disposed
475+
// TraceWriter. Might happen if a function tries to
476+
// log after calling done()
477+
}
478+
}
479+
480+
return Task.FromResult<object>(null);
481+
});
482+
}
483+
484+
private static ScriptFunc CreateMetricFunc(WeakReference<FunctionInvocationContext> invocationContextReference)
485+
{
486+
return (ScriptFunc)(p =>
487+
{
488+
var metricObject = (IDictionary<string, object>)p;
489+
string name = (string)metricObject["name"];
490+
object valueObject = metricObject["value"];
491+
492+
if (name != null && valueObject != null)
493+
{
494+
double value = Convert.ToDouble(valueObject);
495+
FunctionInvocationContext invocationContext = null;
496+
if (invocationContextReference.TryGetTarget(out invocationContext))
497+
{
498+
using (BeginFunctionScope(invocationContext))
499+
{
500+
var properties = (IDictionary<string, object>)metricObject["properties"];
501+
invocationContext.Logger.LogMetric(name, value, properties);
502+
}
503+
}
504+
}
505+
506+
return Task.FromResult<object>(null);
507+
});
508+
}
509+
498510
private static IDisposable BeginFunctionScope(FunctionInvocationContext context)
499511
{
500512
// Node captures the AsyncLocal value of the first invocation, which means that logs

‎src/WebJobs.Script/azurefunctions/functions.js‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ function createFunction(f) {
5555
origMetric({ name: name, value: value, properties: properties});
5656
};
5757

58-
context.done = function (err, result) {
58+
context.done = function (err, returnValue) {
5959
if (context._done) {
6060
if (context._promise) {
6161
context.log("Error: Choose either to return a promise or call 'done'. Do not use both in your script.");
@@ -70,16 +70,23 @@ function createFunction(f) {
7070
callback(err);
7171
}
7272
else {
73-
var values = {};
7473
if (context.res && context.bindings.res === undefined) {
7574
context.bindings.res = context.res;
7675
}
76+
77+
// because Edge.JS interop doesn't flow new values added to objects,
78+
// we capture the binding values and pass them back as part of the
79+
// result
80+
var bindingValues = {};
7781
for (var name in context.bindings) {
78-
values[name] = context.bindings[name];
82+
bindingValues[name] = context.bindings[name];
7983
}
80-
context.bind(values, function (err) {
81-
callback(err, result);
82-
});
84+
85+
var result = {
86+
returnValue: returnValue,
87+
bindingValues: bindingValues
88+
};
89+
callback(null, result);
8390
}
8491
};
8592

0 commit comments

Comments
 (0)