Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incompatible with Bottleneck #69

Closed
timdp opened this issue Jun 12, 2016 · 12 comments
Closed

Incompatible with Bottleneck #69

timdp opened this issue Jun 12, 2016 · 12 comments

Comments

@timdp
Copy link

timdp commented Jun 12, 2016

I'm having an issue where Lolex seems to be incompatible with Bottleneck, a popular rate-limiting package. Bottleneck schedules callbacks using setTimeout, and if I mock those calls using Lolex, only the first callback gets called for some reason.

I initially thought it might be a Bottleneck issue, but then I proceeded to replace Lolex with my own API-compatible implementation, and the problem went away (for my example code).

Maybe I'm misunderstanding Lolex. At any rate, thanks.

@fatso83
Copy link
Contributor

fatso83 commented Jun 12, 2016

Not sure what you are using lolex for. Lolex is supposed to take control over setTimeout and friends. That is its purpose, so that you can have full control over timers. But it is not a mocking or stubbing library: that is the work of sinon. If you want to control time, but not let lolex affect the internal workings of your library you need to cache references to the globals you are using. That means you need to do something like this in your internal code, possibly wrapped in a closure:

(function myLib(global){
    const setTimeout = global.setTimeout
    global.myLib = {};

    // stuff happens

    setTimeout(foobar, 1000); // will use your internal timeout reference

    // other stuff happens
})(window);

Closing this issue as a use-related issue. Not seeing an issue with lolex here.

@fatso83 fatso83 closed this as completed Jun 12, 2016
@timdp
Copy link
Author

timdp commented Jun 12, 2016

I'm aware of the need for caching. I think you misread the issue description. This is what happens:

  1. clock = lolex.install()
  2. setTimeout(fn1, 0) (from Bottleneck)
  3. setTimeout(fn2, 0) (from Bottleneck)
  4. clock.tick(1)

If fn2 doesn't get called, isn't that a bug?

@timdp
Copy link
Author

timdp commented Jun 12, 2016

Maybe take a look at my original example as well. The calls to limiter.schedule() make Bottleneck perform the setTimeout that seems to be causing trouble.

@fatso83 fatso83 reopened this Jun 12, 2016
@fatso83
Copy link
Contributor

fatso83 commented Jun 12, 2016

Thanks for the code example. That is a much clearer description. "Callback did not get called" could mean a whole lot of things, and I assumed you referred to something else than you example code did. This does indeed look like a bug ...

@timdp
Copy link
Author

timdp commented Jun 12, 2016

I apologize for the confusion. Probably should've copy-pasted my entire explanation from the Bottleneck issue rather than linking to it.

Anything I can do to help debug? Bottleneck is originally in CoffeeScript, making it a little clunky to debug, but since injecting my own Lolex-compatible code fixed it, I still think we should be looking at Lolex code.

@fatso83
Copy link
Contributor

fatso83 commented Jun 12, 2016

@timdp Help is always appreciated. You could try cloning the repo and add some tests to see if you are able to trigger it in a clean environment. https://github.com/sinonjs/lolex/blob/master/test/lolex-test.js#L305 is supposed to cover your case, but perhaps we have an issue with the special case of time=0?

@SGrondin
Copy link

A setTimeout of 0 is supposed to be equivalent to setImmediate, and same thing for a setTimeout of a negative number.

@timdp
Copy link
Author

timdp commented Jun 13, 2016

@SGrondin Does Bottleneck use negative numbers?

I was thinking it might be due to something like setTimeout(() => setTimeout(f, 0), 0). That, or the combination of setTimeout and promises (bluebird or native) acting up, especially if your Promise uses setImmediate or process.nextTick internally.

Anyway, I'll test soon.

@SGrondin
Copy link

SGrondin commented Jun 13, 2016

Bottleneck doesn't call a setTimeout inside of another setTimeout (but your user code might) and it never calls setTimeout with a negative number. By default, it uses Bluebird if it's installed in your project and native promises otherwise. But even internally, it's just basic callbacks, it just returns a promise to the outside world so you can chain it. There's no setImmediate or nextTick anywhere in the code either.

@timdp
Copy link
Author

timdp commented Jun 16, 2016

Still trying to get to the bottom of this, but what I've found so far is that core-js (added by Babel) insists on using its own Promise implementation in my transpiled code because most promises don't fully adhere to the spec. My goal is to replace every Promise with pinkie, which uses setImmediate, which I can delegate to lolex.

@fatso83
Copy link
Contributor

fatso83 commented Jul 15, 2016

Looked into this a bit. Not sure what is happening, but there is one thing that sticks out. No new timers are scheduled after the first call completes. You can check this with clock.timers.

@fatso83
Copy link
Contributor

fatso83 commented Jul 13, 2017

Although we don't have a good test code to show this problem, and so this has not been verified to be an actual Sinon problem, I suspect that the async changes in #102 and #105 might fix the issue. As we have very little to go by I am going to close this as missing information required to dig into this, but it would be cool if @timdp could check out the new version of lolex with the asyncTick feature to see if that fixes things (in a few days).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants