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

Already on GitHub? Sign in to your account

basic `guard` functionality #96

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants

sakari commented Feb 8, 2013

A way to control when a deferred is run for e.g. throttling.
See #94

Owner

briancavalier commented Feb 8, 2013

Thanks @sakari, I'll try to dig into this over the weekend! @riccieri, I'll be interested to hear your thoughts as well, when you have time to look at this.

Contributor

renato-zannon commented Feb 8, 2013

Hey, this is promising (no pun intended) :) I'll take a look at this over the weekend as well!

Owner

briancavalier commented Feb 11, 2013

I really like this functionality, especially since it is so modular and doesn't require modifying the existing when.map et. al., or creating new variants of them.

One main, overall thought is that when.js is more functional in style, rather than using constructors+prototypes. I'd like to stick with that style for this, if possible. For example, I can imagine the guard function being written as:

function guard(condition, functionToGuard) {
    // return a guarded version of functionToGuard
}

This would still allow people to create a guard "instance" using partial application:

// Create a new function called guardFactory that can be used to create
// functions that have a concurrency limit of 3
var guardFactory = guard.bind(undefined, 3);

var myGuardedFunction = guardFactory(myUnguardedFunction);

There will be extra closures needed to implement it that way, but that doesn't bother me.

Thoughts?

Sorry for not having a CONTRIBUTING document set up :( In addition to being MIT Licensed, all contributions will become copyright Brian Cavalier & John Hann as per the standard cujojs copyright header. I hope that's not a problem, but please do let me know if it is.

@renato-zannon renato-zannon and 1 other commented on an outdated diff Feb 11, 2013

test/guard.js
+resolved = when.resolve;
+rejected = when.reject;
+
+
+buster.testCase('when/guard', {
+ 'should be usable for throttling when.map': function(done) {
+ var called = [];
+ var dfds = [when.defer(), when.defer(), when.defer(), when.defer()];
+ when.map([0, 1, 2, 3], guard(2).do(function(v) {
+ called.push(v);
+ return dfds[v];
+ }));
+ assert.equals(called, [0, 1]);
+ dfds[1].resolve();
+ assert.equals(called, [0, 1, 2]);
+ done();
@renato-zannon

renato-zannon Feb 11, 2013

Contributor

An important note: On future versions, the resolution of when's promises will always happen on the next turn (via setImmediate, process.nextTick etc), so these tests would probably fail. To be future proof, it's better to not count on handlers being called synchronously with the resolve() call. An alternative could be something like this:

when.map([0, 1, 2, 3], guard(2).do(function(v) {
  // Current code
})).then(function() {
  // First assertion
  dfds[1].resolve();
  return dfds[1].promise;
}).then(function() {
  // Second assertion
}).always(done);
@briancavalier

briancavalier Feb 11, 2013

Owner

Nice catch @riccieri. Yes, we've found that using .always() is quite convenient for this.

Hmmm, I'd like to find an alternative to using strings for t. Here are a couple of thoughts; let me know what you think:

  1. Use a boolean. Since there are only 2 possible values, it's a decent match. However, that seems less obvious than using 'enter' and 'exit' :/
  2. Instead of condition being a function, could it be an object: { acquire: function() {}, release: function() {} } (or enter/exit depending on your terminology preference!). I think this makes it very clear, and means not having to use if/then inside the condition.

I'm torn. On one hand, I like condition being just a function, but on the other hand I like the clarity and lack of if/then of 2.

FWIW, I like the clarity of 2. I think it goes on the same vein of callbacks.promisify: Support the version with more ceremony first, and later add shorthands if people ask for them.

Please include a contact email address.

sakari commented Feb 12, 2013

functionGuard

If we do not have a separate Guard object we cannot so easily share guards between functions.

var g = guard(1);
var f_g = g.do(f);
var w_g = g.do(w);

The equivalent would require passing the guard condition around which on the first glance is not so nice if you just want to throttle it with a shorthand numeral. Also doing equivalent of g.trigger() for building more complex scheduling schemes would be harder.

condition object

I can never remember whether up semaphor means that it's taken or free... I would like to avoid true and false for this reason.
Generally I'd like to think of this in terms of entering a region and exiting from it instead of acquiring an imaginary lock.

Allowing a condition object would be fine -- it might even be better than a function. We can support both.

nextTicks

Darn. Nice to know. Testing asynchronous stuff is way more tricky.

Well I think that I could manage with doing some kind of invariant to ensure that the scheduling works in the tests.

Timing

When are you going to release the next version from master?

Contributor

renato-zannon commented Feb 12, 2013

@sakari, I think you can use partial application for that:

var g   = guard.bind(null, 1);
var f_g = g(f);
var w_g = g(w);

does this make sense?

sakari commented Feb 12, 2013

Surely above the guard is not shared between f_g and w_g using the standard javascript Function.bind i.e.

f_g(); w_g();

executes both guarded functions immediately.

Owner

briancavalier commented Feb 12, 2013

Here's my current thinking:

functionGuard

@sakari it is possible to use partial application, but the code needs to be refactored for it to work. If you'd prefer to stick with the prototype request for this pull request, that's fine with me, and we can move forward as is. Before releasing it, though, we may decide to refactor it into a partial application-based functional style.

condition object

I think we should support only: 1) numbers, and 2) an object using @sakari's preferred terminology: { enter: function, exit: function }. Let's not support a function right now. If it becomes obvious later that supporting a function is something that users want, we can easily add that.

nextTicks

Take a look at other unit tests for examples of how to test async promises. In most cases, it's pretty easy, and means simply doing assertions inside the promise chain, and then ending the promise chain with .always(done) to inform BusterJS that the test is done. Of course, there are always weird cases where that isn't sufficient! We're happy to help and answer questions if you need.

Timing

We will probably release 1.8.0 (currently the dev branch) within the next week. If we don't get this pull request merged by then, that's ok. We can always do a 1.8.1 release shortly after :)

This is the use case for which I do not really see an solution with guard.bind(null, 1). Of course changing the api would make it possible.

Right, it would definitely require the API to be different, and the code to be refactored slightly.

Well, this is going to seem weird, and I apologize! We've decided that the right thing to do going forward is to allow people to have the option of retaining copyright to contributed modules if they want, as long as they use an MIT license.

So, if you'd like to retain the copyright, please use the following:

/** @license MIT License (c) copyright original author or authors */

Then, below, include your real name or the name of the organization if you'd prefer that your org retains the copyright (or mandates it), and optionally, an email in the @author. See example below.

Again, I apologize for the bad timing, and for any confusion! We'll be publishing a CONTRIBUTING doc soon for all of cujojs, and it will detail all of this.

For example: * @author Sakari Jokinen <sakari@rocketpack.fi>

You can use BusterJS's spies, stubs, and mocks, along with their set of "called" assertions to do this kind of test, rather than recording the calls in an array.

See note above about using BusterJS's spies and spy/stub assertions for this.

In general, it's more bullet-proof to use .always(done) at the end of a test. That will ensure done in all possible cases.

Use always(done)

always(done)

always(done)

always(done)

sakari commented Feb 21, 2013

@briancavalier, thanks for the feedback. As you may have guessed I'm not that familiar with buster :).

I'm now on a paternity leave so from my part this will have to wait a few weeks. @kschzt might be able to work on this if he has time. If you do not hear from him I'll get back to this when I get back to office.

Owner

briancavalier commented Feb 21, 2013

Hey @sakari, thanks for the update, and congratulations! :)

How would you feel about this: I'll add a functional version of guard() to when.js based on your concepts, and give you attribution? The attribution will list you as @contributor plus some language like: "adapted from original concept by Sakari Jokinen (Rocketpack, Ltd)"

Let me know if that's ok with you, and if any of your colleagues should also be included in the attribution. Thanks!

sakari commented Feb 23, 2013

Thanks

@briancavalier that would be awesome. It's Rocket Pack with a space in between. Rocketpack is a company in Australia which I learned shortly after updating my linkedin profile once ;)

Owner

briancavalier commented Feb 23, 2013

@sakari Great! I've create #116 to track the development of the functional version--so closing this one.

Enjoy your time off with family!

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