technomancy / robert-hooke

Hooke your Clojure functions!
Other
359 stars 25 forks source link

with-hooks-disabled thread visibility and f meta #16

Closed narkisr closed 10 years ago

narkisr commented 10 years ago

Current implementation of with-hooks-disabled makes use of with-redefs that are visible to all running threads,

This makes its use in multi threaded code unexpected (some threads will have it disabled while other don't depends on with-redefs call duration).

Using with-bindings makes it effect only single thread visible and in my mind more usable.

Second commit adds the metadata back to the f argument making it possible for the hook to operate on its name and argument, without it hooks are left only with analyzing args structure limiting its use.

Thanks

technomancy commented 10 years ago

Changing this to use binding will make it only work on vars that have been declared dynamic.

narkisr commented 10 years ago

True, but without it the function will result with unexpected behavior in multi threaded settings,

And there is also this, where an aspect might get removed completely and not restored,

I'm well aware to the burden and performance hit of declaring dynamic vars but the use case for removing hooks/aspects in run time isn't common.

I think that a reasonable compromise is to require dynamic vars in case you plan to remove them or at least add a warning to the docs, I was bitten by this.

Thanks

technomancy commented 10 years ago

The primary use case for this library is to add functionality to vars that you don't control. If you have the power to make the var dynamic, then it's probably a var that's under your control, in which case you'd be better off adding real extensibility to it rather than forcing a change from the outside via add-hook.

Happy to take a patch that documents the thread-unsafety of with-hooks-disabled though.