Skip to content

Commit

Permalink
Fix out-of-sync binding names causing null ref error (#216)
Browse files Browse the repository at this point in the history
* Move toCoreFunctionMetadata to separate file

* Use string hash instead of count for binding names
  • Loading branch information
ejizba authored Feb 5, 2024
1 parent 02f7ba3 commit 3822bc3
Show file tree
Hide file tree
Showing 5 changed files with 247 additions and 67 deletions.
17 changes: 8 additions & 9 deletions src/addBindingName.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the MIT License.

const bindingCounts: Record<string, number> = {};
import { getStringHash } from './utils/getRandomHexString';

/**
* If the host spawns multiple workers, it expects the metadata (including binding name) to be the same across workers.
* That means we need to generate binding names in a deterministic fashion, so we'll do that using a count
* There's a tiny risk users register bindings in a non-deterministic order (i.e. async race conditions), but it's okay considering the following:
* 1. We will track the count individually for each binding type. This makes the names more readable and reduces the chances a race condition will matter
* 2. Users can manually specify the name themselves (aka if they're doing weird async stuff) and we will respect that
* That means we need to generate binding names in a deterministic fashion, so we'll do that using a string hash of the binding data
* A few considerations:
* 1. We will include the binding type in the name to make it more readable
* 2. Users can manually specify the name themselves and we will respect that
* 3. The only time the hash should cause a conflict is if a single function has duplicate bindings. Not sure why someone would do that, but we will throw an error at function registration time
* More info here: https://github.com/Azure/azure-functions-nodejs-worker/issues/638
*/
export function addBindingName<T extends { type: string; name?: string }>(
Expand All @@ -19,10 +21,7 @@ export function addBindingName<T extends { type: string; name?: string }>(
if (!bindingType.toLowerCase().endsWith(suffix.toLowerCase())) {
bindingType += suffix;
}
let count = bindingCounts[bindingType] || 0;
count += 1;
bindingCounts[bindingType] = count;
binding.name = bindingType + count.toString();
binding.name = bindingType + getStringHash(JSON.stringify(binding));
}
return <T & { name: string }>binding;
}
60 changes: 2 additions & 58 deletions src/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ import {
CosmosDBFunctionOptions,
EventGridFunctionOptions,
EventHubFunctionOptions,
ExponentialBackoffRetryOptions,
FixedDelayRetryOptions,
FunctionOptions,
FunctionTrigger,
GenericFunctionOptions,
Expand All @@ -21,14 +19,11 @@ import {
TimerFunctionOptions,
WarmupFunctionOptions,
} from '@azure/functions';
import * as coreTypes from '@azure/functions-core';
import { FunctionCallback } from '@azure/functions-core';
import { returnBindingKey } from './constants';
import { toRpcDuration } from './converters/toRpcDuration';
import { toCoreFunctionMetadata } from './converters/toCoreFunctionMetadata';
import * as output from './output';
import { ProgrammingModel } from './ProgrammingModel';
import * as trigger from './trigger';
import { isTrigger } from './utils/isTrigger';
import { tryGetCoreApiLazy } from './utils/tryGetCoreApiLazy';

export * as hook from './hooks/registerHook';
Expand Down Expand Up @@ -138,63 +133,12 @@ export function generic(name: string, options: GenericFunctionOptions): void {
setProgrammingModel();
}

const bindings: Record<string, coreTypes.RpcBindingInfo> = {};

const trigger = options.trigger;
bindings[trigger.name] = {
...trigger,
direction: 'in',
type: isTrigger(trigger.type) ? trigger.type : trigger.type + 'Trigger',
};

if (options.extraInputs) {
for (const input of options.extraInputs) {
bindings[input.name] = {
...input,
direction: 'in',
};
}
}

if (options.return) {
bindings[returnBindingKey] = {
...options.return,
direction: 'out',
};
}

if (options.extraOutputs) {
for (const output of options.extraOutputs) {
bindings[output.name] = {
...output,
direction: 'out',
};
}
}

let retryOptions: coreTypes.RpcRetryOptions | undefined;
if (options.retry) {
retryOptions = {
...options.retry,
retryStrategy: options.retry.strategy,
delayInterval: toRpcDuration((<FixedDelayRetryOptions>options.retry).delayInterval, 'retry.delayInterval'),
maximumInterval: toRpcDuration(
(<ExponentialBackoffRetryOptions>options.retry).maximumInterval,
'retry.maximumInterval'
),
minimumInterval: toRpcDuration(
(<ExponentialBackoffRetryOptions>options.retry).minimumInterval,
'retry.minimumInterval'
),
};
}

const coreApi = tryGetCoreApiLazy();
if (!coreApi) {
console.warn(
`WARNING: Skipping call to register function "${name}" because the "@azure/functions" package is in test mode.`
);
} else {
coreApi.registerFunction({ name, bindings, retryOptions }, <FunctionCallback>options.handler);
coreApi.registerFunction(toCoreFunctionMetadata(name, options), <FunctionCallback>options.handler);
}
}
76 changes: 76 additions & 0 deletions src/converters/toCoreFunctionMetadata.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the MIT License.

import { ExponentialBackoffRetryOptions, FixedDelayRetryOptions, GenericFunctionOptions } from '@azure/functions';
import * as coreTypes from '@azure/functions-core';
import { returnBindingKey } from '../constants';
import { AzFuncSystemError } from '../errors';
import { isTrigger } from '../utils/isTrigger';
import { toRpcDuration } from './toRpcDuration';

export function toCoreFunctionMetadata(name: string, options: GenericFunctionOptions): coreTypes.FunctionMetadata {
const bindings: Record<string, coreTypes.RpcBindingInfo> = {};
const bindingNames: string[] = [];

const trigger = options.trigger;
bindings[trigger.name] = {
...trigger,
direction: 'in',
type: isTrigger(trigger.type) ? trigger.type : trigger.type + 'Trigger',
};
bindingNames.push(trigger.name);

if (options.extraInputs) {
for (const input of options.extraInputs) {
bindings[input.name] = {
...input,
direction: 'in',
};
bindingNames.push(input.name);
}
}

if (options.return) {
bindings[returnBindingKey] = {
...options.return,
direction: 'out',
};
bindingNames.push(returnBindingKey);
}

if (options.extraOutputs) {
for (const output of options.extraOutputs) {
bindings[output.name] = {
...output,
direction: 'out',
};
bindingNames.push(output.name);
}
}

const dupeBindings = bindingNames.filter((v, i) => bindingNames.indexOf(v) !== i);
if (dupeBindings.length > 0) {
throw new AzFuncSystemError(
`Duplicate bindings found for function "${name}". Remove a duplicate binding or manually specify the "name" property to make it unique.`
);
}

let retryOptions: coreTypes.RpcRetryOptions | undefined;
if (options.retry) {
retryOptions = {
...options.retry,
retryStrategy: options.retry.strategy,
delayInterval: toRpcDuration((<FixedDelayRetryOptions>options.retry).delayInterval, 'retry.delayInterval'),
maximumInterval: toRpcDuration(
(<ExponentialBackoffRetryOptions>options.retry).maximumInterval,
'retry.maximumInterval'
),
minimumInterval: toRpcDuration(
(<ExponentialBackoffRetryOptions>options.retry).minimumInterval,
'retry.minimumInterval'
),
};
}

return { name, bindings, retryOptions };
}
4 changes: 4 additions & 0 deletions src/utils/getRandomHexString.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,7 @@ export function getRandomHexString(length = 10): string {
const buffer: Buffer = crypto.randomBytes(Math.ceil(length / 2));
return buffer.toString('hex').slice(0, length);
}

export function getStringHash(data: string, length = 10): string {
return crypto.createHash('sha256').update(data).digest('hex').slice(0, length);
}
157 changes: 157 additions & 0 deletions test/converters/toCoreFunctionMetadata.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the MIT License.

import 'mocha';
import { expect } from 'chai';
import { output, trigger } from '../../src';
import { toCoreFunctionMetadata } from '../../src/converters/toCoreFunctionMetadata';

describe('toCoreFunctionMetadata', () => {
const handler = () => {};
const expectedHttpTrigger = {
authLevel: 'anonymous',
methods: ['GET', 'POST'],
type: 'httpTrigger',
name: 'httpTrigger433d175fc9',
direction: 'in',
};
const expectedHttpOutput = { type: 'http', name: 'httpOutput9a706511b1', direction: 'out' };
const expectedQueueOutput1 = {
queueName: 'e2e-test-queue-trigger',
connection: 'e2eTest_storage',
type: 'queue',
name: 'queueOutput8b95495f3d',
direction: 'out',
};

it('http trigger', () => {
const result = toCoreFunctionMetadata('funcName', {
handler,
trigger: trigger.http({}),
return: output.http({}),
});
expect(result).to.deep.equal({
name: 'funcName',
bindings: {
httpTrigger433d175fc9: expectedHttpTrigger,
$return: expectedHttpOutput,
},
retryOptions: undefined,
});
});

it('http trigger, storage output', () => {
const result = toCoreFunctionMetadata('funcName', {
handler,
trigger: trigger.http({}),
return: output.http({}),
extraOutputs: [
output.storageQueue({
queueName: 'e2e-test-queue-trigger',
connection: 'e2eTest_storage',
}),
],
});
expect(result).to.deep.equal({
name: 'funcName',
bindings: {
httpTrigger433d175fc9: expectedHttpTrigger,
$return: expectedHttpOutput,
queueOutput8b95495f3d: expectedQueueOutput1,
},
retryOptions: undefined,
});
});

it('http trigger, multiple storage output', () => {
const result = toCoreFunctionMetadata('funcName', {
handler,
trigger: trigger.http({}),
return: output.http({}),
extraOutputs: [
output.storageQueue({
queueName: 'e2e-test-queue-trigger',
connection: 'e2eTest_storage',
}),
output.storageQueue({
queueName: 'e2e-test-queue-trigger2',
connection: 'e2eTest_storage',
}),
],
});
expect(result).to.deep.equal({
name: 'funcName',
bindings: {
httpTrigger433d175fc9: expectedHttpTrigger,
$return: expectedHttpOutput,
queueOutput8b95495f3d: expectedQueueOutput1,
queueOutput7ab7ce64ad: {
queueName: 'e2e-test-queue-trigger2',
connection: 'e2eTest_storage',
type: 'queue',
name: 'queueOutput7ab7ce64ad',
direction: 'out',
},
},
retryOptions: undefined,
});
});

it('http trigger, duplicate storage output', () => {
expect(() => {
toCoreFunctionMetadata('funcName', {
handler,
trigger: trigger.http({}),
return: output.http({}),
extraOutputs: [
output.storageQueue({
queueName: 'e2e-test-queue-trigger',
connection: 'e2eTest_storage',
}),
output.storageQueue({
queueName: 'e2e-test-queue-trigger',
connection: 'e2eTest_storage',
}),
],
});
}).to.throw(/duplicate bindings found/i);
});

it('http trigger, duplicate storage output with name workaround', () => {
const result = toCoreFunctionMetadata('funcName', {
handler,
trigger: trigger.http({}),
return: output.http({}),
extraOutputs: [
output.storageQueue({
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
name: 'notADupe',
queueName: 'e2e-test-queue-trigger',
connection: 'e2eTest_storage',
}),
output.storageQueue({
queueName: 'e2e-test-queue-trigger',
connection: 'e2eTest_storage',
}),
],
});

expect(result).to.deep.equal({
name: 'funcName',
bindings: {
httpTrigger433d175fc9: expectedHttpTrigger,
$return: expectedHttpOutput,
queueOutput8b95495f3d: expectedQueueOutput1,
notADupe: {
queueName: 'e2e-test-queue-trigger',
connection: 'e2eTest_storage',
type: 'queue',
name: 'notADupe',
direction: 'out',
},
},
retryOptions: undefined,
});
});
});

0 comments on commit 3822bc3

Please sign in to comment.