remove shimmer, implement new one

[obecny]

Currently the shimmer is a desired way of patching. The problem is that shimmer doesn't support multiple patching. It stores the original into __original and that's it. If it happen that someone else will be using this to patch the function we might accidently be unpatched or we might unpatch it too. What if there are multiple libraries that want to patch the same function. It should be possible that all libraries will be able to correctly patch the function and then correctly unpatch it to the state it was, but it should also support the queue to unpatch correctly
For example

1. original
2. patch 1
3. patch 2
4. patch 3

now let say the library from point 2 want to unpatch. The scenario should look like this

1. Original
2. patch 2
3. patch 3

So even though the patch has been applied the unpatch should not restore to original, but should be aware of other patches and simply remove the patch it added from the queue.
This way all the libraries that are patching and unpatching will never break each other and you will be able to patch and unpatch such things many times without problem.
If there is no such library I think we should create a new one.

[dyladan]

Bonus: we also remove an external dependency

[Flarna]

Is unpatch really a requirement? It will be really hard to implement a bullet prove patch library which works in all cases. Monkey patching is used more often then someone would expect and there are numerous variants how this is done in detail.
Besides the question of restoring the correct function you end up also with in flight objects created with patched version of a module but have to finish their work in an unpatched world.
Using unpatch for testing is fine but I would not allow live unpatch.

There are other reasons why shimmer may be problematic: it has quite some visible side effects like not coping properties to wrapped function or not setting function name/length on wrapped function. Both may break user applications.

example:

const shimmer = require('shimmer');
const fs = require('fs');

function describe(s, fn) {
  console.log(`${s}: name: ${fn.name}, length: ${fn.length}, typeof(fn.native): ${typeof(fn.native)}`);
}

describe('before', fs.realpath);

shimmer.wrap(fs, 'realpath', function (original) {
  return function () {
    return original.apply(this, arguments)
  }
})
describe('after', fs.realpath);

results in

before: name: realpath, length: 3, typeof(fn.native): function
after: name: , length: 0, typeof(fn.native): undefined

[obecny]

@Flarna
There are other reasons why shimmer may be problematic: it has quite some visible side effects like not coping properties to wrapped function or not setting function name/length on wrapped function. Both may break user applications.
I'm happy to hear that you also don't like shimmer. So as mentioned previously I see more reasons why we should get rid of shimmer and why I created this issue. I don't want to discuss the implementation details here as first we need to decide if that's the way to go. And if we decide to get rid of that I'm happy to write the new shimmer / wrapper that should be bullet proof, but before writing and showing how it works and why I believe it will be bullet proof I don't want to go into implementation discussion instead of showing working code.

[dyladan]

What properties need to be copied from the original function?

• name
• length
• native

Should it be unpatchable?

Is it important that we are actually able to fully unpatch and restore the original version of the function, or is it ok to just have the patched version of the function call the original function if the plugin is disabled? This would reduce the complexity of the shim library, but would require plugins to be aware of their enabled/disabled states.

here is an example of what I mean. One example is not unpatchable and relies on the plugin to manage a state. The other is unpatchable and relies on the shim library to restore original functionality.

/**
 * In this version, the original function is never restored.
 * In the case that the plugin is disabled, it just delegates
 * execution to the original function at that time
 */
class NotUnpatchAble {
  // this property would likely be set by the baseplugin so each plugin
  // does not need to implement its own state
  private _state = PluginState.UNPATCHED;

  patch() {
    if (this._state === PluginState.UNPATCHED) {
      patch(this._moduleExports, "methodName", this._getPatchedVersion());
    }
    this._state = PluginState.ENABLED;
  }

  disable() {
    this._state = PluginState.DISABLED
  }

  private _getPatchedVersion() {
    return function (this: any, original: Function) {
      if (PluginState.DISABLED) {
        return original.apply(this, arguments);
      }

      // do stuff
    }
  }
}

/** In this version, the original function is restored */
class UnpatchAble {
  patch() {
    patch(this._moduleExports, "methodName", this._getPatchedVersion());
  }

  disable() {
    unpatch(this._moduleExports, "methodName");
  }

  private _getPatchedVersion() {
    return function (this: any, original: Function) {
      // do stuff
    }
  }
}

enum PluginState {
  UNPATCHED,
  DISABLED,
  ENABLED,
}

[draffensperger]

Is there any use case for un-patching other than disabling a plugin? I can't think of one personally, which leads me to think that that we really need something like a "disable plugin" functionality that could turn off all the plugin specific code besides calling the underlying patched function.

I am personally very much in favor of getting rid of shimmer in favor of a ligher weight alternative, because it allows removal of shimmer as a dependency (one less thing that can become vulnerable or incompatible, one less thing customers need to audit for, etc.)

[dyladan]

@draffensperger I was thinking the same thing wrt not unpatching and just delegating. It seems like unpatch is an unnecessarily dangerous step when it comes to multiple wrapping

[vmarchaud]

Is there any use case for un-patching other than disabling a plugin ?

And specially, disabling it at runtime which i believe is a really really uncommon thing. Generally people implement a toggle via the environment and disable them by restarting each app, which make the unpatch never used.

[Flarna]

In my opinion it's not really possible at all to unpatch everything.

If you patch an API like e.g. fs.read() everyone who imported via const { read } = require("fs") has the monkey patched version cached and unpatch can't change this.

I think the only valid use case for unpatch is for module tests.

[obecny]

it's not only about unpatching but patching too. If any other library will use shimmer on the same function it will remove our patch, it will be exactly the same if someone else used shimmer to patch function and then we will patch it - it will remove someone else patch.

[vmarchaud]

@obecny Totally, that was a issue when i was working at an APM vendor, we couldnt be used at the same time as another. That could be tolerable since generally you don't have multiple APM at the same time.
However for some company that monkey patch for other cases than monitoring (@sqreen for ex) it's really problematic, i believe we should make sure that those product can still works with OT (/cc @vdeturckheim)

[obecny]

this seems to be outdated and unwanted, closing