Regular expression & check function support for checking routes

[cadorn]

See: #67

[rauchg]

Thanks, I was looking at this yesterday and was thinking of simplifying

[cadorn]

@guille

Hmm, this may need a bit more work. In our use-case we register many instances of engine.io at different paths/routes on the same server and are getting this warning:

(node) warning: possible EventEmitter memory leak detected. 11 listeners added. Use emitter.setMaxListeners() to increase limit.
Trace: 
at Socket.<anonymous> (events.js:139:15)
at Server.handleSocket (/home/ubuntu/c9/node_modules/cloud9/node_modules/smith.io/node_modules/engine.io/lib/server.js:280:10)
at HTTPServer.<anonymous> (/home/ubuntu/c9/node_modules/cloud9/node_modules/smith.io/node_modules/engine.io/lib/engine.io.js:160:14)
at HTTPServer.emit (events.js:88:20)
at TCP.onconnection (net.js:876:8)

The problem is here: https://github.com/LearnBoost/engine.io/blob/fefb643c4a6ff77d38571e2bc5832cd0a471f840/lib/engine.io.js#L147

Where many listeners are attached to the same server.

Looks like we need one listener per server and then check routes before delegating to engine.io instance. Any suggestions on how that should best be done?

I would be happy to take a stab at it.

[cadorn]

Alright, all fixed. Let me know what needs to change for this to land.

[rauchg]

It looks good, but attach needs a refactor methinks.
Also, why setMaxListeners(1000) ?

[cadorn]

It looks good, but attach needs a refactor methinks.

It does not look pretty but kind of all belongs into attach. Not sure how to refactor.

Also, why setMaxListeners(1000) ?

We attach many engine.io instances at different paths to same server. If no setMaxListeners we get (node) warning: possible EventEmitter memory leak detected. 11 listeners added.. Using large number as it should not throw warning even if many instances.

[rauchg]

How many instances do you have?

[cadorn]

Not 100% sure but >1k possibly as high as 5k.
We could make it a config option?

[rauchg]

What's the benefit over having one instance with a regexp?

[cadorn]

I suppose its a limitation of our wrapper.

We would need the path along with the socket for engine.on("connection") events: https://github.com/c9/smith.io/blob/master/server-plugin/plugin.js#L29

Can that be done? I have not looked at the engine.io code too closely.

With that change we could lower setMaxListeners to maybe 100 so that multiple engine.io instances per server are still supported in case you want 12+ independent namespaces.

[rauchg]

Yeah, keep in mind per SPEC engine.io also gives flexibility with the query string. You can pass arbitrary data related to the connection there too.

[rauchg]

In a way I would say the "path" is a special engine.io thing, almost not user-land. The main reason for this is that otherwise we're almost introducing a pseudo/limited router in an attach function. It's just not right

[cadorn]

Ok, I'll fix our wrapper and set the limit to 100.

[rauchg]

I'm also curious as to why retaining /engine.io and passing variable identifying information as part of the query string is not a viable option. At the end of the day, the only reason we have the idea of "dynamic path names" in the web today is for user-friendliness, which obviously is not a valid use case for us. Most variable parameters are perfectly fine if set as part of the query string.

[rauchg]

Also in a way you break the "contract" with other software that could be looking at your requests to determine what kind they are. Since we can't set a header "X-Requested-With: Engine" or "X-Engine-IO", the only way external components could know engine is in place is by looking at the path.

For example, I believe http://nodetime.com/ looks for "/socket.io" to give you socket.io analytics.

[rauchg]

@cadorn I think what we should give the flexibility of regular expressions / functions / etc is the resource, not the path. I think I'm going to make the path immutable, in fact.

[cadorn]

We currently only load balance based on the path. I suppose that can be updated to look at the query string as well.

[cadorn]

I'll fix our wrapper to create only one engine.io instance and use the resource instead of path to register a dynamic route. We can then land this and you can make the path immutable.

[cadorn]

Feels much cleaner. Made the path already immutable as it was easier.

[rauchg]

Let's do a smaller pull request first that just adds regular expression / function support for resource, with the appropriate documentation.

Then we'll address the approach to multiple attachments to the same server. This pull request does too much and it's hard to review.

[cadorn]

I can do this but it will take some time.

[rauchg]

@cadorn the test "server close should trigger on client if server does not meet ping timeout" fails a lot, from your other pull request

[cadorn]

Hmm. Seems to be fine for me. Any way I can reproduce your environment? What is the failure message?

[cadorn]

Created separate issues.