Make join throw if called synchronously from a closed spark

[lpinca]

Follow up of #58.
This makes Rooms#join throw an error if it is called without a callback and the spark is closed.

[coveralls]

Coverage increased (+0.002%) to 99.528% when pulling 971a289 on throw/error into bd79aad on master.

[lpinca]

I still see an issue when using batch joins:

primus.join([spark1, spark2, spark3], 'foo bar');

The above example should throw an error if any of the spark is closed, but it doesn't now.
To fix this we should also tweak the batch method.

I wonder if we should change strategy. To handle these errors the developers need to use a try...catch statement or check by themself if the spark/s is/are closed before calling spark.join or primus.join.
My point is that we are only duplicating code that will be in the app anyway.
I wonder if it makes sense to revert 538fc57, close this pr, and only improve the documentation specifying that calling spark.join from a closed spark will result in a memory leak.

[fadeenk]

@lpinca is there a reason why this PR isn't merged??

[lpinca]

It's an incomplete solution. It doesn't work for batch joins.

[lpinca]

As said above, I think the best approach would be to just document the behavior. Calling spark.join from a closed spark causes a memory leak.

[fadeenk]

I agree, however there might be a way of getting this to work.

According to async's documentation

If any of the functions pass an error to its callback, the main callback is immediately called with the value of the error

So instead of removing the throw keep it there and just pass a callback when doing the batch call here

parallel(tasks, cb(res){
 if (res instanceof Error) throw res;
 if (fn) return setImmediate(fn, res);
});

Assuming I am understanding the goal of this PR correctly correctly. The following should work and from what I could tell it is only used when doing join and leave. (it is too early for me right now, so i might have missed something, or misunderstanding what is going on)

[lpinca]

@fadeenk it makes sense. I'll merge this. Are you up for creating a follow up PR for Rooms.prototype.batch()?

[lpinca]

Actually I think I can just do it in this PR, I'm just too lazy to add a test haha.

[lpinca]

Hmm two thoughts:

1. A try...catch does not work if the above approach is used as the error is thrown from the parallel callback

2. There could be cases where some sparks successfully join the specified room/s even if an error is returned, but I think this can happen also when using any adapter which is not the memory adapter.

[fadeenk]

To follow up to last comment

1. as far as I can tell if we are using a batch join there wont be any way to throw an error that can be caught using a try...catch unless we change the batch call from a callback to a promise

2. That is correct, thats maybe something we need to change on how the batch works. Maybe the batch join/leave returns an array with success status or an err?

[lpinca]

as far as I can tell if we are using a batch join there wont be any way to throw an error that can be caught using a try...catch unless we change the batch call from a callback to a promise

If the goal is to check if sparks are closed then yes it's possible.

diff --git a/lib/rooms.js b/lib/rooms.js
index 9662dc8..922c300 100644
--- a/lib/rooms.js
+++ b/lib/rooms.js
@@ -469,16 +469,27 @@ Object.defineProperty(Rooms.prototype, 'connections', {
 
 Rooms.prototype.batch = function batch(method, spark, room, fn) {
   var sparks = Array.isArray(spark) ? spark : [spark]
-    , tasks = [];
+    , tasks = []
+    , i = 0
+    , err;
 
-  fn = fn || noop;
-
-  sparks.forEach(function each(spark) {
-    if ('string' === typeof spark) spark = this.primus.spark(spark);
+  function addTask(spark) {
     tasks.push(function task(done) {
       spark[method](room, done);
     });
-  }, this);
+  }
+
+  for (; i < sparks.length; i++) {
+    spark = this.primus.spark(
+      'string' === typeof spark[i] ? spark[i] : spark[i].id
+    );
+    if (!spark) {
+      err = new RoomsError('One or more closed sparks');
+      if (fn) return setImmediate(fn, err), this;
+      throw err;
+    }
+    addTask(spark);
+  }
 
   parallel(tasks, fn);
   return this;

[lpinca]

I honestly don't know how useful it is. It will raise awareness but to avoid those errors the end users should check if the sparks are closed in their code so we are only duplicating everything.

I also wonder how useful this batch feature is and it if makes sense to remove it so that primus.join and primus.leave will only handle a single spark.

This plugin does too much imo :)

[fadeenk]

The diff you posted looks good, that will not even call the actual join if there is a disconnected socket. That may be good but it might cause issues since the behavior is a bit of opinionated. Another way to implement it would have been to join the connected ones and return the status for each socket join/leave.

Also, the err is handled in the callback however the throw err will just cause an uncaught exception because the batch call is not wrapped in a try...catch

I do agree the batch join/leave kind of doesn't make sense in the scope of the plugin, we are literally just iterating through the sockets and joining them. I feel it would make more sense to just allow for a single join and have the user implement their own logic of how to do and handle multiple sockets joins.

[lpinca]

I feel it would make more sense to just allow for a single join and have the user implement their own logic of how to do and handle multiple sockets joins.

This.

[fadeenk]

Should I just make a PR for that? or should we wait on a response from @cayasso first?

[cayasso]

Hi guys, thanks for bringing this up again, I agree with you all about the batch function, this was one of the initial requirements that I needed for one of my clients. But I have been thinking on a refactoring to make the plugin smaller and more scoped, so in the spirit of simplicity batch can go away, also another thing that I think can go away and we have talked about is sync support in favor of only async. Also move on to promises. These changes of course will be a major release so we would need to document broken changes and also provide great examples.

What do you guys think?

JB

[lpinca]

Works for me. I have a special love for callbacks but can live with promises :)

[cayasso]

I have started some work on refactoring primus-rooms-adapter https://github.com/cayasso/primus-rooms-adapter/tree/1.0 its just a starting point @lpinca feel free to take a look when you have some time, it doesn't have tests or tests doesn't wok yet, also feel free to suggest or add changes ;-)

[cayasso]

@lpinca yea I love callbacks still but I am getting in love of async/await I really don't like the then/catch syntax but adding support for async/await comes with a price in the code size :(

[lpinca]

@cayasso promises are fine, no problem.