Skip to content

Commit 6b38d1c

Browse files
benglbengl
authored
remove safeWrap from shimmer (#5598)
* remove safeWrap from shimmer * don't change non-safe-mode shimmer behaviour
1 parent 34f19d5 commit 6b38d1c

File tree

2 files changed

+3
-329
lines changed

2 files changed

+3
-329
lines changed

‎packages/datadog-shimmer/src/shimmer.js

Lines changed: 3 additions & 128 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
'use strict'
22

3-
const log = require('../../dd-trace/src/log')
4-
53
function copyProperties (original, wrapped) {
64
// TODO getPrototypeOf is not fast. Should we instead do this in specific
75
// instrumentations where needed?
@@ -24,9 +22,7 @@ function copyProperties (original, wrapped) {
2422
function wrapFunction (original, wrapper) {
2523
if (typeof original === 'function') assertNotClass(original)
2624

27-
const wrapped = safeMode
28-
? safeWrapper(original, wrapper)
29-
: wrapper(original)
25+
const wrapped = wrapper(original)
3026

3127
if (typeof original === 'function') copyProperties(original, wrapped)
3228

@@ -37,46 +33,14 @@ const wrapFn = function (original, delegate) {
3733
throw new Error('calling `wrap()` with 2 args is deprecated. Use wrapFunction instead.')
3834
}
3935

40-
// This is only used in safe mode. It's a simple state machine to track if the
41-
// original method was called and if it returned. We need this to determine if
42-
// an error was thrown by the original method, or by us. We'll use one of these
43-
// per call to a wrapped method.
44-
class CallState {
45-
constructor () {
46-
this.called = false
47-
this.completed = false
48-
this.retVal = undefined
49-
}
50-
51-
startCall () {
52-
this.called = true
53-
}
54-
55-
endCall (retVal) {
56-
this.completed = true
57-
this.retVal = retVal
58-
}
59-
}
60-
61-
function isPromise (obj) {
62-
return obj && typeof obj === 'object' && typeof obj.then === 'function'
63-
}
64-
65-
let safeMode = !!process.env.DD_INEJCTION_ENABLED
66-
function setSafe (value) {
67-
safeMode = value
68-
}
69-
7036
function wrapMethod (target, name, wrapper, noAssert) {
7137
if (!noAssert) {
7238
assertMethod(target, name)
7339
assertFunction(wrapper)
7440
}
7541

7642
const original = target[name]
77-
const wrapped = safeMode && original
78-
? safeWrapper(original, wrapper)
79-
: wrapper(original)
43+
const wrapped = wrapper(original)
8044

8145
const descriptor = Object.getOwnPropertyDescriptor(target, name)
8246

@@ -110,94 +74,6 @@ function wrapMethod (target, name, wrapper, noAssert) {
11074
return target
11175
}
11276

113-
function safeWrapper (original, wrapper) {
114-
// In this mode, we make a best-effort attempt to handle errors that are thrown
115-
// by us, rather than wrapped code. With such errors, we log them, and then attempt
116-
// to return the result as if no wrapping was done at all.
117-
//
118-
// Caveats:
119-
// * If the original function is called in a later iteration of the event loop,
120-
// and we throw _then_, then it won't be caught by this. In practice, we always call
121-
// the original function synchronously, so this is not a problem.
122-
// * While async errors are dealt with here, errors in callbacks are not. This
123-
// is because we don't necessarily know _for sure_ that any function arguments
124-
// are wrapped by us. We could wrap them all anyway and just make that assumption,
125-
// or just assume that the last argument is always a callback set by us if it's a
126-
// function, but those don't seem like things we can rely on. We could add a
127-
// `shimmer.markCallbackAsWrapped()` function that's a no-op outside safe-mode,
128-
// but that means modifying every instrumentation. Even then, the complexity of
129-
// this code increases because then we'd need to effectively do the reverse of
130-
// what we're doing for synchronous functions. This is a TODO.
131-
132-
// We're going to hold on to current callState in this variable in this scope,
133-
// which is fine because any time we reference it, we're referencing it synchronously.
134-
// We'll use it in the our wrapper (which, again, is called syncrhonously), and in the
135-
// errorHandler, which will already have been bound to this callState.
136-
let currentCallState
137-
138-
// Rather than calling the original function directly from the shim wrapper, we wrap
139-
// it again so that we can track if it was called and if it returned. This is because
140-
// we need to know if an error was thrown by the original function, or by us.
141-
// We could do this inside the `wrapper` function defined below, which would simplify
142-
// managing the callState, but then we'd be calling `wrapper` on each invocation, so
143-
// instead we do it here, once.
144-
const innerWrapped = wrapper(function (...args) {
145-
// We need to stash the callState here because of recursion.
146-
const callState = currentCallState
147-
callState.startCall()
148-
const retVal = original.apply(this, args)
149-
if (isPromise(retVal)) {
150-
retVal.then(callState.endCall.bind(callState))
151-
} else {
152-
callState.endCall(retVal)
153-
}
154-
return retVal
155-
})
156-
157-
// This is the crux of what we're doing in safe mode. It handles errors
158-
// that _we_ cause, by logging them, and transparently providing results
159-
// as if no wrapping was done at all. That means detecting (via callState)
160-
// whether the function has already run or not, and if it has, returning
161-
// the result, and otherwise calling the original function unwrapped.
162-
const handleError = function (args, callState, e) {
163-
if (callState.completed) {
164-
// error was thrown after original function returned/resolved, so
165-
// it was us. log it.
166-
log.error('Shimmer error was thrown after original function returned/resolved', e)
167-
// original ran and returned something. return it.
168-
return callState.retVal
169-
}
170-
171-
if (!callState.called) {
172-
// error was thrown before original function was called, so
173-
// it was us. log it.
174-
log.error('Shimmer error was thrown before original function was called', e)
175-
// original never ran. call it unwrapped.
176-
return original.apply(this, args)
177-
}
178-
179-
// error was thrown during original function execution, so
180-
// it was them. throw.
181-
throw e
182-
}
183-
184-
// The wrapped function is the one that will be called by the user.
185-
// It calls our version of the original function, which manages the
186-
// callState. That way when we use the errorHandler, it can tell where
187-
// the error originated.
188-
return function (...args) {
189-
currentCallState = new CallState()
190-
const errorHandler = handleError.bind(this, args, currentCallState)
191-
192-
try {
193-
const retVal = innerWrapped.apply(this, args)
194-
return isPromise(retVal) ? retVal.catch(errorHandler) : retVal
195-
} catch (e) {
196-
return errorHandler(e)
197-
}
198-
}
199-
}
200-
20177
function wrap (target, name, wrapper) {
20278
return typeof name === 'function'
20379
? wrapFn(target, name)
@@ -256,6 +132,5 @@ function assertNotClass (target) {
256132
module.exports = {
257133
wrap,
258134
wrapFunction,
259-
massWrap,
260-
setSafe
135+
massWrap
261136
}

‎packages/datadog-shimmer/test/shimmer.spec.js

Lines changed: 0 additions & 201 deletions
Original file line numberDiff line numberDiff line change
@@ -189,207 +189,6 @@ describe('shimmer', () => {
189189
it('should validate that the method wrapper is a function', () => {
190190
expect(() => shimmer.wrap({ a: () => {} }, 'a', 'notafunction')).to.throw()
191191
})
192-
193-
describe('safe mode', () => {
194-
let obj
195-
196-
before(() => {
197-
shimmer.setSafe(true)
198-
})
199-
200-
after(() => {
201-
shimmer.setSafe(false)
202-
})
203-
204-
describe('sync', () => {
205-
beforeEach(() => {
206-
obj = { count: () => 3 }
207-
})
208-
209-
it('should not throw when wrapper code is throwing', () => {
210-
shimmer.wrap(obj, 'count', () => {
211-
return () => {
212-
throw new Error('wrapper error')
213-
}
214-
})
215-
216-
expect(obj.count()).to.equal(3)
217-
})
218-
219-
it('should not throw when wrapper code is throwing after return', () => {
220-
shimmer.wrap(obj, 'count', (count) => {
221-
return () => {
222-
count()
223-
throw new Error('wrapper error')
224-
}
225-
})
226-
227-
expect(obj.count()).to.equal(3)
228-
})
229-
})
230-
231-
describe('sync recursive', () => {
232-
beforeEach(() => {
233-
obj = { count: (x = 1) => x === 3 ? 3 : obj.count(x + 1) }
234-
})
235-
236-
it('should not throw when wrapper code is throwing', () => {
237-
shimmer.wrap(obj, 'count', (count) => {
238-
return function (x) {
239-
if (x === 2) {
240-
throw new Error('wrapper error')
241-
}
242-
return count.apply(this, arguments)
243-
}
244-
})
245-
246-
expect(obj.count()).to.equal(3)
247-
})
248-
249-
it('should not throw when wrapper code is throwing mid-recursion', () => {
250-
shimmer.wrap(obj, 'count', (count) => {
251-
return function (x) {
252-
const returnValue = count.apply(this, arguments)
253-
if (x === 2) {
254-
throw new Error('wrapper error')
255-
}
256-
return returnValue
257-
}
258-
})
259-
260-
expect(obj.count()).to.equal(3)
261-
})
262-
263-
it('should not throw when wrapper code is throwing after return', () => {
264-
shimmer.wrap(obj, 'count', (count) => {
265-
return function (x) {
266-
const returnValue = count.apply(this, arguments)
267-
if (x === 3) {
268-
throw new Error('wrapper error')
269-
}
270-
return returnValue
271-
}
272-
})
273-
274-
expect(obj.count()).to.equal(3)
275-
})
276-
})
277-
278-
describe('async', () => {
279-
beforeEach(() => {
280-
obj = { count: async () => await Promise.resolve(3) }
281-
})
282-
283-
it('should not throw when wrapper code is throwing', async () => {
284-
shimmer.wrap(obj, 'count', (count) => {
285-
return async function (x) {
286-
if (x === 2) {
287-
throw new Error('wrapper error')
288-
}
289-
return await count.apply(this, arguments)
290-
}
291-
})
292-
293-
expect(await obj.count()).to.equal(3)
294-
})
295-
296-
it('should not throw when wrapper code is throwing after return', async () => {
297-
shimmer.wrap(obj, 'count', (count) => {
298-
return async () => {
299-
await count()
300-
throw new Error('wrapper error')
301-
}
302-
})
303-
304-
expect(await obj.count()).to.equal(3)
305-
})
306-
})
307-
308-
describe('async recursion', () => {
309-
beforeEach(() => {
310-
obj = {
311-
async count (x = 1) {
312-
if (x === 3) return await Promise.resolve(3)
313-
else return await obj.count(x + 1)
314-
}
315-
}
316-
})
317-
318-
it('should not throw when wrapper code is throwing', async () => {
319-
shimmer.wrap(obj, 'count', (count) => {
320-
return async function (x) {
321-
if (x === 2) {
322-
throw new Error('wrapper error')
323-
}
324-
return await count.apply(this, arguments)
325-
}
326-
})
327-
328-
expect(await obj.count()).to.equal(3)
329-
})
330-
331-
it('should not throw when wrapper code is throwing mid-recursion', async () => {
332-
shimmer.wrap(obj, 'count', (count) => {
333-
return async function (x) {
334-
const returnValue = await count.apply(this, arguments)
335-
if (x === 2) {
336-
throw new Error('wrapper error')
337-
}
338-
return returnValue
339-
}
340-
})
341-
342-
expect(await obj.count()).to.equal(3)
343-
})
344-
345-
it('should not throw when wrapper code is throwing after return', async () => {
346-
shimmer.wrap(obj, 'count', (count) => {
347-
return async function (x) {
348-
const returnValue = await count.apply(this, arguments)
349-
if (x === 3) {
350-
throw new Error('wrapper error')
351-
}
352-
return returnValue
353-
}
354-
})
355-
356-
expect(await obj.count()).to.equal(3)
357-
})
358-
})
359-
// describe('callback', () => {
360-
// it('should not throw when wrapper code is throwing', (done) => {
361-
// const obj = { count: cb => setImmediate(() => cb(null, 3)) }
362-
363-
// shimmer.wrap(obj, 'count', () => {
364-
// return () => {
365-
// throw new Error('wrapper error')
366-
// }
367-
// })
368-
369-
// obj.count((err, res) => {
370-
// expect(res).to.equal(3)
371-
// done()
372-
// })
373-
// })
374-
// it('should not throw when wrapper code calls cb with error', async () => {
375-
// const obj = { count: cb => setImmediate(() => cb(null, 3)) }
376-
377-
// shimmer.wrap(obj, 'count', (count) => {
378-
// return (cb) => {
379-
// count((err, val) => {
380-
// cb(new Error('wrapper error'))
381-
// })
382-
// }
383-
// })
384-
385-
// obj.count((err, res) => {
386-
// expect(err).to.be.undefined
387-
// expect(res).to.equal(3)
388-
// done()
389-
// })
390-
// })
391-
// })
392-
})
393192
})
394193

395194
describe('with a function', () => {

0 commit comments

Comments
 (0)