subscribing fails with a function that doesn't have a normal bind()

[getify]

[Update: the conversation has shifted down-thread, from this OP being about a "broken" subscriber callback, to whether or not there's any merit to being able to opt-out of bind(..)ing, for performance reasons]

---

Describe the bug

I have a unique situation where I'm trying to subscribe to an observable with a function that has an overridden bind(..), which does something different than normal Function#bind would do. I tried passing that function as a subscription next function, and subscribe(..) either seems to ignore it (silently) or in some other way just fails to ever call it as expected.

Expected behavior

I expected to be able to pass in any function that is normally callable, like fn(..) (or fn.call(..) or fn.apply(..)) and have it work fine as a subscription.

As shown in the reproduction code below, the simple workaround is to wrap my function in an arrow/lambda. That's not awful, but it's annoying and a bit wasteful to have to do so. I'd really like to avoid that.

I wouldn't have expected that subscriber functions must be this-bindable. I read through some documentation and I didn't see such a gotcha listed -- apologies if this is documented and I missed it.

If it's not documented, I think it probably should be!

My argument against the current behavior is, a lot of people probably pass => functions anyway, so Rx is likely not getting much out of trying to bind the this context on those subscription function calls.

---

But if that behavior is just absolutely required by RxJS, I wonder if there could be some way of opting out that doesn't involve having to wrap my function in an arrow/lambda every time? Perhaps, for example, it could be something like one of these:

rx.subscribe({ _next: myA });   // or some other variation of property name

// or
rx.subscribe({ next: myA, noBind: true });

// or
rx.subscribeNoBind({ next: myA });

It could ALSO possibly be that having a path to opt-out of this binding -- for those like me don't need or want it -- might even be just ever so slightly faster on execution. ;-)

Reproduction code

import { of } from 'rxjs';

function myA(v) { console.log("a",v); }
function myB(v) { console.log("b",v); }

myA.bind = function(){ /*.. something different ..*/ }
myB.bind = function(){ /*.. something different ..*/ }

const rx = of(42);

// does not work
rx.subscribe({ next: myA });

// works fine
rx.subscribe({ next: v => myB(v) });

Reproduction URL

https://stackblitz.com/edit/rxjs-y1wqko

Version

7.5.2

Environment

Node 16

Additional context

The source of these special functions (that don't have a normal behaving bind(..)) is from a library I wrote, and I want users to be able to use them with RxJS as easily as possible.

[kwonoj]

I'm not sure if I understood this correctly.

So, this occurs

1. A function deliberately override prototype.bind which makes a function does not behave as documented spec behavior of prototype.bind
2. and if observable receives those as one of observer callback it will not work as observable internally relies on prototype.bind.

Am I reading this correct? Given observer's callback requires a function, I would say it implicitly requires given function should behave as function which is spec behavior for any of prototype behavior. I may miss something but didn't get fully why bind needs to be treated specifically.

[getify]

Am I reading this correct?

Yes.

I would say it implicitly requires given function should behave as function

It's a normal function that's fully callable in all ways (fn(), new fn(), fn.call(..), fn.apply(..)). It's just not a function that can vend a new this-bound version of itself via fn.bind(..).

note: arrow functions (which are extremely common in JS) are also not bindable... it's just that bind(..) exists on arrow functions and is a wasted no-op. For my library's functions, the bind(..) is overridden to do something different.

What I'm perplexed about is why RxJS needs to be able to bind such functions anyway?

But, if Rx really needs to be able to do so for some use-cases, is there an affordance that Rx can offer so users can opt out to avoid the this-binding part... that wouldn't even have to be default, so no breakage of existing code.

And bonus: if you opt-out and skipped the this-binding of your subscription, that likely would perform ever-so-slightly more efficiently.

[kwonoj]

Other core team members might have a different opinion, but in my opinion, this is internal implementation detail of a, or any library that relies on standard behavior of javascript. Yes, as you said bind itself may have slightly different characteristics other than some spec behavior of fn - but that doesn't mean any code / library should be able to provide an escape hatch if consumer intentionally choose to override its behavior to not follow intended specs. I'm a bit lost to understand why RxJS should provide an escape hatch, or explicitly explain it relies on certain spec behavior internally for these cases.

It may possible further version of RxJS could remove usage of bind for some other reasons. If that happens, that may resolve your usecases but I don't expect we'll refactor code specifically because of this reason.

[getify]

but I don't expect we'll refactor code specifically because of this reason

If you like, forget for a moment that it's a "breakage" I'm experiencing. Pretend my request is an additional feature purely from the justification that I want to skip the "unnecessary" (albeit very slim) performance overhead of bind(..) being called on my function, when I know my function is not this-aware and therefore the this-binding is pointless.

I'm a bit lost to understand why RxJS should provide an escape hatch

Anyone who passes => arrow functions in (I suspect a lot of devs!) can now opt-out of pointless bind(..)ing and may trim a dozen microseconds off each subscribe/dispatch.

---

SIDE NOTE: As I was poking around in the code to propose what changes would be necessary, I found this note which seems to indicate that at least part of the reason for this binding of subscription next handlers was for a now deprecated access to this.unsubscribe(..)... a feature that's now behind a flag and going to be removed (in version 8.x I guess?) Perhaps this-binding is going to stick around past that, or perhaps all the this binding is going away? I wasn't clear from what it says.

---

In any case, if we think about this issue as a request for a new feature in version 8, I'm imagining it would involve minor changes in two spots, the subscribe(..) function and the SafeSubscriber(..) constructor:

1. the subscribe(..) function needs to look in the "partial observer" object passed into it for something like a _next or noBind or skipBind present, and if so, pass an additional boolean to the SafeSubscriber(..) constructor:

    subscribe(
       observerOrNext?: Partial<Observer<T>> | ((value: T) => void) | null,
       error?: ((error: any) => void) | null,
       complete?: (() => void) | null
     ): Subscription {
   +    const skipBind = (observerOrNext.skipBind === true);
   -    const subscriber = isSubscriber(observerOrNext) ? observerOrNext : new SafeSubscriber(observerOrNext, error, complete);
   +    const subscriber = isSubscriber(observerOrNext) ? observerOrNext : new SafeSubscriber(observerOrNext, error, complete, skipBind);

2. A new parameter (called skipBind or whatever) in the SafeSubscriber(..) constructor signature, which defaults to false:

   subscribe(
       observerOrNext?: Partial<Observer<T>> | ((value: T) => void) | null,
       error?: ((error: any) => void) | null,
   -   complete?: (() => void) | null
   +   complete?: (() => void) | null,
   +   skipBind = false
     ): Subscription {

   ....and a single conditional in the SafeSubscriber constructor:

   - next = next?.bind(context);
   + next = skipBind ? next : next?.bind(context);

I may be missing other details, but I think that seems like it would essentially cover what I'm suggesting.

[kwonoj]

1. I do not think we can skip this request is coming from your code's specific usecases. I still think this is very specific to implementation detail of a library.
2. As I already mentioned, we may, or may not decide to stop using bind, but I do not believe we'll explicitly exposing it as public level.
3. I do not agree proposed api surface exposure, again, in my opinion this is very close to implementation detail. First, what'll happen if we internally decide to not to use bind at all? What happens in public api surface? Also, what'll happen if we choose other mechanism which potentially bring similar questions? What is actual performance overhead we are currently seeing? Do we have proven performance overhead worth enough to expose additional api surface? Especially given general direction where RxJS attempts to proceed is reduce & simplify api surfaces as much.

Overall, unless I miss some points this request asks to change internals of library, or expose those internals of libaray into public api surface without noticeable gain we can actually achieve. I'd happy to be proven wrong if I could see 1. bind actually creates lot of overhead 2. Therefore, we'd like to remove it in the future version but in any case I don't think I'll agree to expose those into public api surface level.

[getify]

in my opinion this is very close to implementation detail

When I posted this issue I didn't know that the this-binding of subscriber next functions was a thing. It seemed like an internal-only implementation detail. After poking around on this topic, it's now clear that it's an intentional, user-facing feature.

First, what'll happen if we internally decide to not to use bind at all?

Since it's a real feature, it would be a disruptive breaking change for you to later remove this feature from Rx. Even if you did decide to make such a breaking change, you'd be deprecating it for a release or two and then later actually removing it. In that case, you'd also be deprecating and removing the skipBind or whatever it would be called.

Even if, hypothetically, you didn't deprecate and gracefully remove the this-binding feature, but just made a quick breaking change to remove it (for some outlandish reason)... the hanging skipBind in user code would then just be ignored and pointless, but not breaking... it's not like having introduced that flag would lock you into a behavior you didn't want to keep -- since you (hypothetically) removed this-binding, you no longer care if people want to opt-out of a now-non-existent behavior!

What I'm suggesting is intended to opt-OUT of a feature, not OPT-IN to some other feature. My point: since it's a user-facing "feature", it's not just an internal implementation detail.

I do not think we can skip this request is coming from your code's specific usecases

I think it's unfortunate that you're unwilling to shift mental contexts. Had I never mentioned anything about breaking functions and was only talking about the performance optimization opt-out, such a hang-up wouldn't be relevant to the discussion.

What is actual performance overhead we are currently seeing?

That's a perfectly good and fair question. I don't have a concrete answer to that, but I can tell you that bind(..) has historically had performance costs in various JS engines, so even if it's better now, it's not zero.

But it would indeed be interesting to benchmark what sort of performance improvement occurs if the bind(..) is skipped. I'm not nearly familiar enough with Rx to accurately do that with your library (testing all its various entry paths). But I could certainly construct a bare-bones JS benchmark for whether putting a pointless bind() call in costs anything, and moreover whether every call to the pointlessly-bound function is any noticeably slower than just directly calling the function. In fact, I'm sure others have done so on many occasions -- the research for bare/native JS probably already exists!

However, absent such research, it seems you're already jumping to the assumption/conclusion -- that it's negligible -- before we've looked into it.

Moreover, I think we should think of the performance implications from two angles:

1. The docs in multiple places suggest people do things like subscribe({ next: v => whatever(..), ... }). IOW, it's actually suggested that people pass => arrow functions in as these subscriber callbacks; certainly a lot of developers are thus doing so.

   That means NONE of them are benefitting from the this-binding that's happening. Whatever micro/milliseconds are being spent on the bind(..) for all of them, it's being wasted (and as we know, small things add up to big things when done a lot). Some of those folks might be happy to see a release-note in a future version that says they can simply add a boolean flag and get a minor speed up with no other code changes.

2. In the other direction, there's probably some folks out there who are passing their own this-aware functions in as subscriber callbacks. For them, the this-binding that Rx does may actually be an "anti-feature" in that their functions may rely on their own this and not want Rx messing with it. For those folks, they've previously had two options:

   • wrap their callbacks in a lambda (thereby wasting an extra call on the call-stack, AND wasting the bind(..))
   • do their own bind(..) on the function before they pass it in (thereby wasting the extra bind(..))

   In either case, they're presently "working around" Rx's behavior, which is costing a tiny performance hit to do so. I bet many of them would love a note in a future release that allowed them to pass a boolean flag and get rid of their work-arounds.

Perhaps it's at least worth discussing if such a minor performance-oriented feature might serve some folks well.

[kwonoj]

I think fundamental different point of view is what you consider as user-facing, non implementation detail is in my opinion it is implementation detail that a library can make necessary changes anytime. To say this is user facing, in my opinion I believe there should be feasible user story that this can break their application code when code follows spec.

I think I have expressed my own opinion enough and do not think it's ok to repeat mine again. Issue will leave as opened for now in case if any other core member want to share their opinions.

[getify]

To say this is user facing, in my opinion I believe there should be feasible user story that this can break their application code when code follows spec.

Whether it's documented or not, the fact that every single subscriber callback is this-bound to the subscriber, makes it a user-facing feature. There must be code out there that is relying on that, and will break if Rx removed bind(..) internally for some reason.

In fact, the code comment I referenced above, which talks about intentionally deprecating the access to this.unsubscribe(..), is proof that this is considered a "feature"; if it was only an implementation detail, there'd be no need to deprecate it.

I think I have expressed my own opinion enough

Me too. I think this should be considered as a feature-request. But Rx isn't my library. I don't need to spend any more time advocating for it. If y'all decide to reject the request, c'est la vie. Sorry if I've been disruptive here.

[benlesh]

I see the issue and it's a minor fix.

That said, people shouldn't be patching known methods on functions. Haha.

[benlesh]

I actually have two different PRs for different approaches to this #6796 and #6797.

That said, this is a highly unusual ask. Basically, @getify, you're saying "please don't rely on the behavior of Function.prototype.bind because someone could patch it". Given that Function is one of the most primitive types in JavaScript, if we can't rely on its methods, I'm not sure what we can rely on.

I'm not sure what the performance implications are for either of my PRs. I feel like the second one is closest to parity with the original code, however the first one with the arrow functions will be cleaner in the 8.x branch (where we've done away with a deprecated behavior related to the whole bind thing).

[benlesh]

Just to re-hash in the simplest terms. Whatever bind calls are done in RxJS v7, are done specifically so we don't break this on supplied object methods when we try to wrap the functions to deal with unhandled errors (an RxJS feature). Basically just ensuring this works:

source$.subscribe({
  next(value) {
     this.myMethod(value);
  },
  myMethod(value) {
     console.log(value);
  }
})

The this.unsubscribe() stuff is deprecate and a red herring.

However, what is being posed as a bug/issue here is something that would break with even the simplest function that relied on Function.prototype.bind's behavior:

function logThisMessage() {
   console.log(`Message`, this.message);
}

// This is going to break everything.
logThisMessage.bind = () => { /* lol */ };

function useHiThere(fn) {
  return fn.bind({ message: 'hi there' });
}

useHiThere(logThisMessage)();

So I guess what I'm going to ask, @getify, can you present a case why it's reasonable for any library to assume that users have passed in a function with a broken bind method? I don't mind the fix, honestly, I just want to make sure we're not accommodating a corner-case we shouldn't be.

[getify]

@benlesh

Basically, @getify, you're saying "please don't rely on the behavior of Function.prototype.bind because someone could patch it"

That's not what I'm asking. I'm not asking you to change any of your assumptions about how people use your library, nor any default behaviors. Even if I don't use or care about those behaviors, I assume some people do, and that's fine.

I am however observing that even your documentation most commonly provides non-this-aware functions (as arrow functions), so I think there's a substantial portion of users, myself included, who are providing your library non-this-aware functions. For those us doing so, I was suggesting/requesting an additional affordance: to opt-out of your library doing the this binding. That's it, plain and simple.

If I provide you a non-this-aware function (like an arrow function), you calling bind(..) on it is totally pointless and wasteful (even a tiny bit of CPU). If I have a this-aware function that needs a different this than the one your library is assuming to bind(..) for me, then I have to do my own bind(..) calls, in which case your library calling bind(..) on an already bound function is, again, totally pointless and wasteful (even a tiny bit of CPU).

There must be a good chunk of Rx users in one of those two buckets... so on behalf of them, I was asking for the ability to opt-out of that path. How to opt-out is entirely subjective bikeshedding and I don't really much care what such an opt-out would look like.

So I guess what I'm going to ask, @getify, can you present a case why it's reasonable for any library to assume that users have passed in a function with a broken bind method?

Speaking of red herrings, I definitely regret mentioning this fact in the subject/OP, as it's not at all the point anymore. As noted several times, now that I understand the situation better, I have shifted my attention away from "it's broken!" to "can I have an opt-out?". Perhaps I should have just closed this thread and opened a new fresh one without the prior baggage.

But again, the simple "use case" is: I am providing you a function which does not need or want your bind(..) call performed against it, and I'd like to tell you that you can and should skip doing so. It has nothing to do with whether such unnecessary binding would break or not. It's just about avoiding unnecessary operations.

[getify]

@benlesh I'm reluctant to indulge your actual question directly, as its focus on my previous-but-now-set-aside motivations may further distract and invite bikeshedding of its own.

However, for curiosity/posterity sake, I will explain the kind of functions that led me to even discover that Rx was doing this binding -- I never knew it did that in all my years interacting with the lib! But I'm gonna put the rest of my answer below, so it's not as tempting to get off into the weeds any further.

The way I discovered that Rx does this binding of subscribe callbacks is by providing a function to Rx that intentionally has replaced the normal bind(..) function with one that does a very different kind of task to this binding. This kind of function will never be this-aware and therefore has absolutely no reason for this-binding. It's also a unary function, so it would never need the partial-application capabilities that built-in bind(..) provides.

So why did I override the name bind(..) instead of picking a different method name? There's a very strong reason to do so. Trust me, I didn't make the decision lightly. I know the downsides.

The special kind of functions I'm talking about are created by my library: Monio. Monio is a monads-in-JS library.

As you may be aware, the concept of monads has one central capability associated with it, which often goes by one of several very common names in the FP world: bind(..), chain(..), or flatMap(..). Regrettably, there's no standard name for this behavior, so depending on your experiences, you may be used to or prefer any of those 3 (or something else entirely!) My informal surveying estimates that more than 90-95% of all monad implementations, especially in JS, use one of those 3 names.

Monio chose, 2+ years ago when I first wrote it, and in an attempt to be as widely approachable as possible, to use all 3 names as aliases, so you can pick whichever your favorite name is for this core monadic behavior.

I didn't ever anticipate at the original time of design that the monad instances created by Monio would be anything other than normal objects, so the method names I put on it were never subject to a collision issue. However, I knew that bind(..) and flatMap(..) were both commonly used method names in other parts of JS, such as on functions and arrays.

Well, as it so happens, a few months ago, I designed a new monad kind for Monio, and these instances of this monad kind are... functions. Not monads holding a function as a value, but literally the monad instance itself is a function.

Now of course, I have a collision issue between monadic bind(..) and JS function's bind(..). What to do!?!?

1. Do I break all of Monio usage by removing bind(..) from all monad kinds? It's far too late for that sort of breaking change, given that Monio has been around (and used in production) for 2+ years.
2. Do I leave these new functions having the built-in bind(..) method? That's problematic if someone is used to using the name bind(..) with all other monad kinds in Monio, and then for this new kind the bind(..)` method isn't what they expect!
3. Do I delete the bind(..) method name entirely? Tempting, since these functions-as-monad-instances will never need this-binding or partial application. But again, creates the same problem for those using bind(..) on other monad kinds and not being able to use bind(..) on this kind of monad.
4. Do I overwrite the bind(..) method to alias the monadic bind? Seems like the more appropriate move for Monio and its users. For completeness sake, I actually back-up the original bind(..) as _bind(..) on the function, just in case in some extraordinary case some really had to have the "real" bind.

OK, so option 4 is what I chose. It's not perfect, but it's better than (2) and (3) which create an inconsistency. And (1) is a really extreme route that I don't wanna do.

Here's where things get more interesting... the new monad kind I've invented for Monio is a special kind of monad that is basically mimicking, or acting as a companion to.... observables. They're called IOx (as in "Reactive IO"). In fact, I have a fromObservable(..) method on this new monad kind, which is designed to consume an Rx observable and turn it into an IOx, and of course the other direction too, toObservable(..) is an obvious and useful tool.

My point? These IOx monads are actually designed and intended that people will use them interactively with Rx observables in their code base. So it's very likely that one of them could end up being used as a subscription callback to Rx.

Writing tests for my library is exactly how I found this current incompatibility. It's obviously quite ironic that in trying to make my library more friendly to Rx users, I discovered this gotcha that makes them ultimately somewhat-less compatible.

So then I started wondering if there was some way for Rx to add a small additional opt-out feature, which would smooth over this incompatibility. As I illustrated above, it's not really an option for Monio to rename its methods or create inconsistencies in its design.

Ultimately, if this request is denied, it'll just make it a little more error prone for folks to use my library with Rx. That bums me out, but it's not the end of the world.

[benlesh]

I see. Well, for our part, we need to do something to execute user-provided observer handlers for next, error, and complete in a try-catch so we can forward errors and prevent them from unwinding the stack and wreaking havoc.

This is actually something that was born out of TC39 discussions on the matter. "Producer interference". Imagine a single branch of a multicast synchronously errors. If it unwinds the stack back to the loop doing to cast, everyone breaks.

There's a few solutions out producer interference. Schedule each omission from observable, schedule at the multicast, or handle the error and forward it. The first solution was a no-go as it breaks a lot of important behaviors and makes it impossible to model EventTarget. The second solution still breaks expectations in some cases (I could discuss this more, but I don't want to write a novel here, haha), so the last choice is our only option.

All of that said, the use of bind is really more of an implementation detail. And if there's a compelling reason to move away from it, like interop with a reasonably popular library, we should investigate it. We just need to take performance into consideration, as this is a hot path area. (Although bound functions aren't necessarily "fast", haha. There's been a huge improvement in perf for them in the last ~7 years)

[getify]

@benlesh thanks for the further explanation. I agree that being able to run user-supplied functions in a try..catch is important -- I generally do similar things in my libraries.

If you don't mind a bit further explanation, I'm curious why the this binding of these functions is necessary for that?

[benlesh]

It's really not required for anything. It's one approach to ensuring we don't break what this means inside of each of the functions passed with the observer. Short-term one of the two PRs I have would do. Longer-term, I'd like to re-architect things in this area to help perf a little. To that end, I have some local spikes for version 8.

Anyhow, this all requires careful consideration. I would rather not introduce new features, as we end up supporting them for years after. Ideally any solution would "just work" and be generally beneficial for the community.

[benlesh]

FWIW: This issue has me thinking about a better architecture in general for Subscriber going forward, and I'm pretty excited about it. But I think it's going to be something we'll have to try to get to in version 8, not version 7.

[benlesh]

@getify FYI: the fix for this is published with 7.5.3

[benlesh]

Also this should work with rxjs@next (8.0.0-alpha.1)