Immediate disconnect breaks worker's task.end

[amv]

I am doing workers that do one task and then exit. In effect I want to do this:

client.registerWorker( 'success-function', {}, function( task ) {
    task.end("success!");
    client.disconnect();
} );

However when I do this, the worker never seems to send a success message to the gearman server.

I hackishly solved the problem by adding "nextTick" timeout before disconnecting:

client.registerWorker( 'success-function', {}, function( task ) {
    task.end("success!");
    setTimeout( function() { client.disconnect(); }, 0 );
} );

Do you think this would warrant a fix? I'm personally not sure wether it is evident for everyone that calling task.end does not actually put the response to the current buffers, and that calling disconnect at the end of the handler causes the contents of the buffers to be checked before the worker function can put it's stuff to the queue (which I would think causes this behaviour, haven't checked yet).

Maybe just moving the disconnect queue checking to nextTick would help?

Is there anything I could do to help with this?

[iarna]

Well, I think task.end's behavior is typical of Node.js streams– it's not something directly in the control of this library.

But that said, I would expect client.disconnect() to not cut us off early.

Putting disconnect() in a setImmediate in socket.js is one possibility, but it feels a little fitzy to me.

You could write that as:

client.registerWorker( 'success-function', {}, function( task ) {
    task.end("success!", function () {
        client.disconnect();
    });
} );

Or with the same meaning:

client.registerWorker( 'success-function', {}, function( task ) {
    task.end("success!");
    task.on("finish", function () {
        client.disconnect();
    });
} );

This is just standard streams stuff though.

Hmm, ok, I think what I think this should do is:

If you call disconnect() then it will wait for all of your tasks to have finish callbacks before actually disconnecting. It will also abort any tasks that haven't called end themselves yet (eg, send FAIL/EXCEPTION replies for them).

How does that sound?

[amv]

If disconnect can work that way, it sounds really good.

I haven't worked a lot with streams yet -- do they have an easily inspectable internal state, which can be used to detect wether end has been called or finish callback have been fully(?) executed? Is this kind of internal state inspection a normal practice with streams?

[amv]

Actually.. What I am aiming to achieve with this is that the worker would execute it's job only once.. So is there a way to "disable" the worker somehow or should this jst be a part of the disconnect call? I would not want the worker to post a GRAB_JOB message to the server as part of it's finish callback just before disconnecting..

I guess i could unregister my function after execution but it sounds a bit wasteful to send unregister messages for a worker that is just about to disconnect anyway..

Not sure if any of this is relevant.. Just voicing my concerns :)

[iarna]

I haven't worked a lot with streams yet -- do they have an easily inspectable internal state, which can be used to detect wether end has been called or finish callback have been fully(?) executed? Is this kind of internal state inspection a normal practice with streams?

They're not inspectable directly. We'd register our own finish listener and keep track of that ourselves.

Regarding one-time-use workers…

I think it might be valuable to have a "maximumTotalJobs" option, so the worker can shut itself down after hitting that number. For your case, you could set that to 1.

Beyond having it stop grabbing jobs when it hits that total I'd have it unregister. While for your use case that's perhaps slightly redundant, it seems pretty harmless to me.

To do this, I think I'd also want the object returned by registerWorker/registerWorkerStream to be an EventListener that emits an event when that limit gets hit, to allow something like:

  client.registerWorker('…', {maxTotalJobs: 50}, function (…) {…})
    .on('unregister', function () { client.disconnect() })

[amv]

I think maxTotalJobs sounds good as a feature. Attaching disconnect on 'unregister' however feels a bit awkward as I would expect the unregister event to be fired instantly when the last job starts processing, and thus would expect you to know that you can call disconnect before actually ending your jobs. Not sure if this is super relevant.

I am in favour of making the disconnect work that way however, maybe with a large timeout (that can be passed to client.disconnect as a parameter?) for disconnecting also in case of lingering/crashed tasks that never signaled end properly. We could also have a "forceInstantDisconnect" parameter (or a separate function) to expose what happens after the timeout.

[iarna]

I'm less keen on a timeout. I think you should just use setTimeout if that's what you want. But I'm definitely for having both soft and hard disconnects. (I'm not fond of flags that change the behavior of functions, much prefer having multiple functions.)