Update dependencies

[BridgeAR]

The dependencies were really old, so I thought I'll update them even though my main concern was just node-redis.

There are still two dependencies that have to be updated because they have breaking changes. Both are probably good for this library.

One is should that will throw on '42' === 42 from version 4.x.x on and the other one is node-redis 2.0. In both cases the tests won't pass anymore. In some tests there are strings returned instead of numbers, so should breaks. And some other tests fail with node-redis 2.0, since it'll return errors that were earlier suppressed (Uncaught Error: LPUSH can't be processed. The connection has already been closed.).

So I highly recommend to check if everything is working as it should.

I'm not even sure if everything works with the dependencies as I updated them, but at least the tests pass. I'm not sure what's with the build and test-tdd tasks in the Makefile btw.

[KeKs0r]

+1
I would also support updating node-redis to 2.x. If there are testcases failing, its a question, wether this change than would break BC. (I am not sure if returning different types is considered such a break)

[behrad]

Great! Can we have a PR @KeKs0r ?

[KeKs0r]

I would assume this already is the PR. Probably @BridgeAR should rebase and then we could check test cases.

[BridgeAR]

I thought this would be ready to merge when I opened the merge request. I did not update the breaking libraries and wanted to look into them after updating all the other libs.

But if you want me too, I can do this in here.

[behrad]

there's a large list of updated packages in your PR @BridgeAR That frightened me to accept this in place! I'm not sure yet if everything will work. So

1. We need more tests to check everything and accept this :)
2. Any node_redis focused update (to the latest stable version of that module) with tests are more than welcome.

You can both work on this on a base PR guys, we should just make sure everything is OK

[KeKs0r]

so @behrad: would you prefer single PR's that update single dependencies? Or should we just make sure that all the updates in this PR work?

I will try to merge this PR into my production project and run the kue tests as well as my production tests to see if I can spot some issues.

[behrad]

Or should we just make sure that all the updates in this PR work?

this is feasible and manageable @KeKs0r , that would be great.

Can you also update node_redis to 2.x version? ( We should check upgrade notes and there should be some changes in Kue I guess)

[BridgeAR]

@behrad lodash and redis 1.0 won't break anything. Nib and stylus are about css and I guess they will also not break anything (the functions still work the same, but the generated css will likely differ - but likely in a positive way).

I tried to update to redis ^2 but as I already mentioned in my original post, commands are fired after the connection was closed and before redis 2.0 these commands failed silently. If you want to know anything about further changes, feel free to ask, as I released all versions from 2.0 on ;)

The dev libs are not all that critical in my opinion but the only breaking lib I found is should. It works stricter than before.

[behrad]

Good @BridgeAR So I will merge current PR after @KeKs0r approved his tests here

We will then fire another PR for node_redis 2.x (or follow up #652)

I am surprised why node_redis 2.x complains about connection being closed!? All Kue connections to redis are permanent. there should be a API miss-use OR an environment condition of yours.

[BridgeAR]

@behrad to quit the connection .end was used instead of .quit. In that case the client ends the connection immediately and does not process any fired commands. This was also the case in node_redis < 2.0 but those commands just failed silently.

So I just added another commit to change .end to .quit and all tests pass with the new node_redis version.

[BridgeAR]

@behrad This will likely fix a couple open issues about jobs not being processed.
As far as I see it kue v.0.9.0 changed .quit to .end (ce212ce) and it kept unnoticed, because node_redis < 2.0 did not report unhandled commands after the connection closed.

[behrad]

Great @BridgeAR , that sounds good.

@KeKs0r & me should also test this now...

[KeKs0r]

@behrad: I tested this PR against my production setup and everything seems to work

Although this is not a guarantee, since there might be features that I am not using in my app.

[KeKs0r]

Okay, somehow this after some more changes to my code, I am now also getting:

LPUSH can't be processed. The connection has already been closed.

@BridgeAR: Did you already look into where this is coming from?

[BridgeAR]

@KeKs0r I'll check, if you could provide me with the command that you used to trigger the error. If you still get that error this is likely a race condition in kue or somewhere else in kue a command is used after the connection has already ended (using shutdown).

Or you had a connection loss while processing the command.

But even if you get that error now, it'll be better than before as those errors happened before too, but you were not notified about that fact.

[KeKs0r]

I found that this line is causing the errors:
https://github.com/Automattic/kue/blob/master/lib/queue/worker.js#L271

How I cause the error
I have a Hapi application that on server start initializes Kue and on server shutdown , it also shuts kue down.

In a test case where I do the following:

1. Start Server
2. Stop Server
3. Start Server
4. Stop Server

I see the issue occuring

So when I look at my code, start server should be equivalent to:

    var q = kue.createQueue({
        prefix: 'q',
        redis: {
            port: options.port,
            host: options.host,
            auth: options.password
        }
    });

And stop server:

q.shutdown(500, cb);

[behrad]

Great report @KeKs0r
That lpush is was a workaround for interrupted ZPOPs, to ensure helper list's size is not less than original job's ZSET
We should guard that line to check self.client is connected before running lpush

[BridgeAR]

@behrad I guess adding the guard check should be done in another PR? Or do you want me to add this too?

[behrad]

I guess adding the guard check should be done in another PR?

Sure yes. You can create a new PR, or I will do it after merged this

[BridgeAR]

@behrad ok, in that case I'd wait until this is merged. Do you want me to change anything else or is this ready to merge?

[behrad]

this seems Ok @BridgeAR unless you want to add more tests.