From 9adf4d785036d5f931edf5e261b7652f53819eed Mon Sep 17 00:00:00 2001 From: Yongseok Date: Fri, 20 Dec 2024 22:49:33 +0900 Subject: [PATCH] [#249] Fix InstrumentArrowFunction local frame stack variable capturing bug [#249] Fix InstrumentArrowFunction local frame stack variable capturing bug [#249] Fix InstrumentArrowFunction local frame stack variable capturing bug [#249] Fix InstrumentArrowFunction local frame stack variable capturing bug [#249] Fix InstrumentArrowFunction local frame stack variable capturing bug [#249] Fix InstrumentArrowFunction local frame stack variable capturing bug --- .../callback-interceptor-runner.js | 43 ------------ .../instrument-arrow-function.js | 22 +++++-- lib/instrumentation/instrument-method.js | 65 ++++--------------- .../module/callback-interceptor.js | 19 ------ .../ioredis-send-command-interceptor.js | 3 +- ...redis-internal-send-command-interceptor.js | 3 +- 6 files changed, 34 insertions(+), 121 deletions(-) delete mode 100644 lib/instrumentation/callback-interceptor-runner.js delete mode 100644 lib/instrumentation/module/callback-interceptor.js diff --git a/lib/instrumentation/callback-interceptor-runner.js b/lib/instrumentation/callback-interceptor-runner.js deleted file mode 100644 index 91a7e5c2..00000000 --- a/lib/instrumentation/callback-interceptor-runner.js +++ /dev/null @@ -1,43 +0,0 @@ -/** - * Pinpoint Node.js Agent - * Copyright 2020-present NAVER Corp. - * Apache License v2.0 - */ - -'use strict' - -const localStorage = require('./context/local-storage') - -class CallbackInterceptorRunner { - constructor(trace, localAsyncId, hookFunctionArguments, traceContext) { - this.trace = trace - this.localAsyncId = localAsyncId - this.original = hookFunctionArguments.getOriginal() - this.traceContext = traceContext - } - - run(wrapper, thisArg, argsArray) { - return localStorage.run(this.trace, () => { - if (!this.trace) { - return this.original.apply(thisArg, argsArray) - } - - if (!wrapper) { - wrapper = {} - } - - if (typeof wrapper.prepareBeforeTrace === 'function') { - wrapper.prepareBeforeTrace() - } - - const childTraceBuilder = this.traceContext.continueAsyncContextTraceObject(this.trace.getTraceRoot(), this.localAsyncId.nextLocalAsyncId2()) - return localStorage.run(childTraceBuilder, () => { - const result = this.original.apply(thisArg, argsArray) - childTraceBuilder.close() - return result - }) - }) - } -} - -module.exports = CallbackInterceptorRunner \ No newline at end of file diff --git a/lib/instrumentation/instrument-arrow-function.js b/lib/instrumentation/instrument-arrow-function.js index cc36653b..865844b8 100644 --- a/lib/instrumentation/instrument-arrow-function.js +++ b/lib/instrumentation/instrument-arrow-function.js @@ -38,7 +38,7 @@ class InstrumentArrowFunction { }) } - addCallbackInterceptor(interceptor, callerSpanEventRecorder) { + addChildTraceInterceptor(callerSpanEventRecorder) { if (!this.target) { return } @@ -50,14 +50,28 @@ class InstrumentArrowFunction { if (!callerSpanEventRecorder || typeof callerSpanEventRecorder.getNextAsyncId !== 'function') { return } - const asyncId = callerSpanEventRecorder.getNextAsyncId() const target = this.target const method = this.method - const trace = localStorage.getStore() const traceContext = this.traceContext + const trace = localStorage.getStore() + const asyncId = callerSpanEventRecorder.getNextAsyncId() shimmer.wrap(target, method, function (original) { - return interceptor.makeHookCallbackFunction(trace, asyncId, new HookFunctionArguments(target, method, original), traceContext) + return function () { + if (!trace) { + return original.apply(this, arguments) + } + if (!asyncId) { + return original.apply(this, arguments) + } + + const childTraceBuilder = traceContext.continueAsyncContextTraceObject(trace.getTraceRoot(), asyncId.nextLocalAsyncId2()) + return localStorage.run(childTraceBuilder, () => { + const result = original.apply(this, arguments) + childTraceBuilder.close() + return result + }) + } }) } } diff --git a/lib/instrumentation/instrument-method.js b/lib/instrumentation/instrument-method.js index 2b949477..491c2014 100644 --- a/lib/instrumentation/instrument-method.js +++ b/lib/instrumentation/instrument-method.js @@ -73,20 +73,21 @@ class InstrumentMethod { if (recorder && arguments.length > 0 && typeof interceptor.callbackIndexOf === 'function' && typeof interceptor.callbackIndexOf(arguments) === 'number') { const asyncId = recorder.getNextAsyncId() const callbackIndex = interceptor.callbackIndexOf(arguments) - const callback = arguments[callbackIndex] - arguments[callbackIndex] = function () { - try { - interceptor.prepareBeforeAsyncTrace?.(this, arguments) - } catch (error) { - log.error(`arguments: ${error}`) + shimmer.wrap(arguments, callbackIndex, function (original) { + return function () { + try { + interceptor.prepareBeforeAsyncTrace?.(this, arguments) + } catch (error) { + log.error(`arguments: ${error}`) + } + const childTraceBuilder = traceContext.continueAsyncContextTraceObject(trace.getTraceRoot(), asyncId.nextLocalAsyncId2()) + return localStorage.run(childTraceBuilder, () => { + const returnedValue = original.apply(this, arguments) + childTraceBuilder?.close() + return returnedValue + }) } - const childTraceBuilder = traceContext.continueAsyncContextTraceObject(trace.getTraceRoot(), asyncId.nextLocalAsyncId2()) - return localStorage.run(childTraceBuilder, () => { - const returnedValue = callback.apply(this, arguments) - childTraceBuilder?.close() - return returnedValue - }) - } + }) } } catch (error) { // TODO: INTERNAL ERROR logger @@ -107,44 +108,6 @@ class InstrumentMethod { } }) } - - addCallbackInterceptor(interceptor, callerSpanEventRecorder) { - if (!this.target) { - return - } - - if (!this.method || typeof this.method !== 'string' || !this.target[this.method] || typeof this.target[this.method] !== 'function') { - return - } - - if (!callerSpanEventRecorder || typeof callerSpanEventRecorder.getNextAsyncId !== 'function') { - return - } - - const traceContext = this.traceContext - shimmer.wrap(this.target, this.method, function (original) { - return function () { - const trace = localStorage.getStore() - if (!trace) { - return original.apply(this, arguments) - } - const asyncId = callerSpanEventRecorder.getNextAsyncId() - - try { - interceptor.prepareBeforeAsyncTrace?.(this, arguments) - } catch (error) { - log.error(`addCallbackInterceptor: ${error}`) - } - - const childTraceBuilder = traceContext.continueAsyncContextTraceObject(trace.getTraceRoot(), asyncId.nextLocalAsyncId2()) - return localStorage.run(childTraceBuilder, () => { - const returnedValue = original.apply(this, arguments) - childTraceBuilder?.close() - return returnedValue - }) - } - }) - } } module.exports = InstrumentMethod \ No newline at end of file diff --git a/lib/instrumentation/module/callback-interceptor.js b/lib/instrumentation/module/callback-interceptor.js deleted file mode 100644 index aa843cb0..00000000 --- a/lib/instrumentation/module/callback-interceptor.js +++ /dev/null @@ -1,19 +0,0 @@ -/** - * Pinpoint Node.js Agent - * Copyright 2020-present NAVER Corp. - * Apache License v2.0 - */ - -'use strict' - -const CallbackInterceptorRunner = require('../callback-interceptor-runner') - -class CallbackInterceptor { - makeHookCallbackFunction(trace, localAsyncId, hookFunctionArguments, traceContext) { - return function () { - return new CallbackInterceptorRunner(trace, localAsyncId, hookFunctionArguments, traceContext).run(null, this, arguments) - } - } -} - -module.exports = CallbackInterceptor \ No newline at end of file diff --git a/lib/instrumentation/module/ioredis/ioredis-send-command-interceptor.js b/lib/instrumentation/module/ioredis/ioredis-send-command-interceptor.js index 55b94611..b5916c08 100644 --- a/lib/instrumentation/module/ioredis/ioredis-send-command-interceptor.js +++ b/lib/instrumentation/module/ioredis/ioredis-send-command-interceptor.js @@ -10,7 +10,6 @@ const MethodDescriptorBuilder = require('../../../context/method-descriptor-buil const serviceType = require('../redis/redis-service-type') const { addressStringOf } = require('../../../utils/convert-utils') const InstrumentArrowFunction = require('../../instrument-arrow-function') -const CallbackInterceptor = require('../callback-interceptor') const endTraceSymbol = Symbol('ioredis-end-trace') class IoredisSendCommandInterceptor { @@ -39,7 +38,7 @@ class IoredisSendCommandInterceptor { const command = args[0] command[endTraceSymbol] = function() {} - InstrumentArrowFunction.make(command, endTraceSymbol, this.traceContext).addCallbackInterceptor(new CallbackInterceptor(), recorder) + InstrumentArrowFunction.make(command, endTraceSymbol, this.traceContext).addChildTraceInterceptor(recorder) command.promise.finally(command[endTraceSymbol]) } } diff --git a/lib/instrumentation/module/redis/redis-internal-send-command-interceptor.js b/lib/instrumentation/module/redis/redis-internal-send-command-interceptor.js index 26f32fca..2c25b893 100644 --- a/lib/instrumentation/module/redis/redis-internal-send-command-interceptor.js +++ b/lib/instrumentation/module/redis/redis-internal-send-command-interceptor.js @@ -10,7 +10,6 @@ const serviceType = require('./redis-service-type') const MethodDescriptorBuilder = require('../../../context/method-descriptor-builder') const { addressStringOf } = require('../../../utils/convert-utils') const InstrumentArrowFunction = require('../../instrument-arrow-function') -const CallbackInterceptor = require('../callback-interceptor') class RedisInternalSendCommandInterceptor { constructor(traceContext) { @@ -40,7 +39,7 @@ class RedisInternalSendCommandInterceptor { } const command = args[0] - InstrumentArrowFunction.make(command, 'callback', this.traceContext).addCallbackInterceptor(new CallbackInterceptor(), recorder) + InstrumentArrowFunction.make(command, 'callback', this.traceContext).addChildTraceInterceptor(recorder) } }