Support dynamic namespaces

[dylanlingelbach]

This is another attempt to fix #1854. It is much more limited in its approach (no automatic disconnects and no host support - as in #1865).

I'm new to the code base so not sure if this is the right approach - open to feedback on how I'm doing this. The questions I have with my approach:

• Does it make sense to set up another middleware path for this? Or should I allow unknown namespaces to flow through to the existing middleware path and then tear down the connection if the namespace isn't found (or is disallowed)?
• Should I pass more than just the name of the namespace to give the server more discretion in allowing the new namespace?
• Is there a better name than useNamespace for adding the middleware? Name feels odd but couldn't think of anything better.

Thanks to @davidbau for #1865 which got me going in the right direction.

[defunctzombie]

This approach looks more promising over other dynamic namespace approaches I have seen here.

[juan77]

+1, seems great and simple.

[defunctzombie]

needs a more descriptive name

[dylanlingelbach]

Changed to npsValidators. Let me know if that works.

[defunctzombie]

@rase- @rauchg overall this code looks like a simple and good approach. I would cleanup some stuff stylistically, but I can do that after we decide if we are merging this in or not.

[defunctzombie]

This helps us for the cloud project because we need to be able to support arbitrary namespace connections there.

[nkzawa]

👍 This approach looks great to me too.

[rase-]

I like it. 👍

[dylanlingelbach]

@defunctzombie - I'm glad you like this. I'm happy to cleanup anything stylistically as well. If there is a style guide or examples to look at let me know.

[davidbau]

I like the simple approach too.

But a question: with this approach, is there a clean way to implement namespace timeouts or cleanup when there are no client connected to a namespace anymore? For applications (like mine) that support a large number of mostly-unused namespaces, we need a way to avoid leaking them when they go unused.

[defunctzombie]

@dylanlingelbach mostly renaming that array of functions. Another stylistic thing I noticed was just return early from if blocks instead of if..else nested sections. Otherwise looks good.

Do you have thoughts around what @davidbau said on cleaning up namespaces when clients disconnect? If we can't tap into a disconnect event, then we could have a reaper timer that just cleans up.

Does anyone see a downside to automatically cleaning up unused namespaces?

[dylanlingelbach]

@defunctzombie - ok, cool, I'll take a stab at cleaning up the stylistic stuff today.

The downsides I see to automatic cleanup are more complex code and less flexibility around when cleanups happen. What if one wants to keep currently unused namespaces around and another doesn't?

How about Server.prototype.removeNamespace that would clean up the namespace and remove it from the server? And a disconnect event on the namespace when a client disconnects? Applications could implement whatever namespace clean up scheme they like.

The approach I outline above definitely puts more burden on the application to clean up but I think the code would be simpler and more flexible. If the vast majority of applications want automatic cleanup then I think it makes sense to have a more automatic cleanup mechanism.

Thoughts?

I'm happy to code this up if people like the approach. Also happy to code it up if people want to take a deeper look.

[dylanlingelbach]

I was working on cleaning up/adding some tests and may have hit an issue with this approach. On a dynamic namespace connection the connect and connection events aren't emitted on the server side if a client is present prior to the server creating the namespace.

Additionally the middleware for the namespace doesn't run because the server had no way to add it.

I'm not sure if this will be an issue in practice. My feeling is the connect/connection events should probably be raised once the server creates the dynamic namespace, but middleware shouldn't run.

Or is it ok that the connect/connection events aren't emitted?

[defunctzombie]

@dylanlingelbach What do you mean by client being present?

[dylanlingelbach]

I mean client connecting to a dynamic namespace prior to the server calling io.of(...) and the Socket being present.

Here is the sequence I am worried about:

1 - Client tries to connect to a namespace the server hasn't created yet:

var ioc = require('socket.io-client');
ioc('localhost', '/dynamic');

2 - The server allows the dynamic namespace which will create the Namespace object on the server
3 - The server creates the namespace so it can send events:

var sio = io(...);
var nsp = sio.of('/dynamic');
nsp.on('connect', function(socket) {
  // Will not be called for /dynamic namespace
});
nsp.use(function(socket, callback) {
  // Will not be called for /dynamic namespace
});

Does that better explain the scenario I'm thinking about?

I'm not sure if the middleware not being called for the existing connection and the connect event not being emitted are something I should worry about.

[defunctzombie]

@dylanlingelbach can you squash into a single commit and rebase on master. It seems some things have changed on master that cause conflicts.

[dylanlingelbach]

@defunctzombie - yep, squashed and rebased.

[dylanlingelbach]

@defunctzombie - anything else you need me to do on this? No rush, just making sure you weren't waiting on me

[defunctzombie]

I think @rauchg needs to have a review of this feature. To me it looks good.

[joeruello]

Any word on this PR?

[dylanlingelbach]

I'm waiting for a code review - I was planning on rebasing after the review.

[CxRes]

@dylanlingelbach Have you created any documentation for this feature? If so can you please direct me to it. Thanks!

[dylanlingelbach]

@CxRes - I have not other than comments in the code. I wasn't sure where to add the documentation. Happy to add it wherever needed.

[CxRes]

@dylanlingelbach: I wouldn't know the best place to add, since I am not sure of how the documentation is being handled. Can anybody from the socket.io team please advise?

My best guess is to make a PR to readme.me file at the root of the project and perhaps a PR on the socket.io-website repo. Sorry for the trouble, but it is feature that appears to fit my use-case and so am itching to test it out.

[leemason]

any progress on this? prs seem slow to merge at the minute, one issue im having is connecting namespaces via an outside source (php laravel) the socket.io server just listens to redis events and emits them on the event.namespace, so the server has no knowledge of the namespace until it emits, and this causes issues if i dont define them upfront.

can we get this added anytime soon?

[CxRes]

@leemason I am at the moment maintaining a fork with @dylanlingelbach code which is rebased on to the most recent master at http://github.com/cxres/socket.io#dynamic-namespaces. The only documentation are the tests, though it is relatively straightforward to use.
I have been after @rauchg for sometime now to include this feature, who said that he will consider adding it in v1.5 but socket.io is still not there yet.

[CxRes]

@leemason Here is a gist with example usage
https://gist.github.com/CxRes/78b9c07c62819214c2e6242089944803.js

[leemason]

@CxRes thanks for sending me that, ive ended up building my own package ontop of engine.io instead as i was spending more time reverse engineering the features than doing what i needed, cheers anyway.

[z639]

So..... Am I right in thinking we still don't have this ability in master ?

[CxRes]

@z639 I do not think socket.io folks care...

[z639]

Annoying... very annoying.

[darrachequesne]

@z639 thanks for bringing that to my attention, I'll try to take a look at the pull request and make the necessary edits by the end of the week.

[z639]

@darrachequesne Thanks for looking into this.

I'm actually still confused as to why this wasn't the default behaviour right from the beginning.

I mean, if the client is sending a valid namespace and nothing is hard-coded at the server, then why wouldn't that namespace be used ?

There's likely a valid reason and that would also explain why this PR was ignored for so long. Any feedback would be great.

[darrachequesne]

Merged as #3187.

[darrachequesne]

Thanks for the hard work!

[CxRes]

@darrachequesne I see that you have rebased #1921. I had made a few changes in a fork because there were a few minor issues with the implementation as I had discussed with @dylanlingelbach above. Could you please look into it and if it appears OK, I could PR those changes as well....

There are essentially 2 issues that I address in the fork:

1. Issue of namespaceConnect event.
2. Returning the reason for a namespace refusal.

[darrachequesne]

@CxRes yes, could you please open a pull request with those changes? A few questions:

• should we include the namespace in the namespaceConnect event?
• how about a newNamespace event instead? I'm thinking of something like:

server.on('newNamespace', (name) => {
  server.of(name).on('connect', /* ... */);
});

[CxRes]

@darrachequesne please give me a bit of time, I get back to this when I have a bit of time on hand.

To answer your questions:

• should we include the namespace in the namespaceConnect event?

The socket arg namespaceConnect (or connect) callback already carries the namespace inside socket.nsp.name. Explicitly returning a name may be more intuitive though!!!

• how about a newNamespace event instead?

Since the name is already available above this seems a bit superfluous. I am not a fan of having a namespaceConnect event honestly but it was the easy way out (See the conversation above on 16 Nov 2015). However, what you are doing is not ideal either because it involves listening for two events which are essentially the same (one always follows the other, making the two events redundant). In an ideal world, just one connect event should suffice with

• the .of() syntax be used only in case of static namespace and
• useNamespaceValidator() is used in case of dynamic namespaces (Perhaps with a sweeter name/alias, maybe even just .of(validatorFn)).

Something like:

server.of(name).on('connect', /* ... */);  // static namespace
server.useNamespaceValidator(validatorFn).on('connect', /* ... */);  // dynamic namespace

What we are doing now is:

server
  .useNamespaceValidator(validatorFn)
  .on('connect', /* for '/' nsp */)
  .on('namespaceConnect', /* for all other nsp */);

This is incidentally why I was especially frustrated with the otherwise good folks here - v2.0 bump would have been the best time to rethink any API changes which could make this feature feel a natural part of socket.io (and the matter was raised at v1.5).

[darrachequesne]

On first thought, server.of((nsp, conn, next) => {/* ... */}).on('connect', /* ... */); sounds great 👍

[darrachequesne]

Would the following make sense?

let dynamicNsp = server.of((nsp, conn, next) => {
  next(null, /^\/dynamic-\d+$/.test(nsp));
}).on('connect', (socket) => {
  let nsp = this;
  nsp.emit('hello');
});

dynamicNsp
  .use(middleware)
  .emit('hello')
  // ...

socket1 asks for connection to /dynamic-101 and connects to both dynamicNsp and dynamic-101 namespaces. So the connect event is emitted twice.

Then socket2 connects to both dynamicNsp and dynamic-102 namespaces.

So dynamicNsp could be used to emit to every sockets in every dynamic-xxx namespaces.

[darrachequesne]

@CxRes and others: I implemented the idea above there #3195, what do you think ?

[CxRes]

@darrachequesne Thanks for your work. I had a look at your code (which is quite different from the 1.x code). In a sense it is really short and sweet so I am reluctant to do suggest too much change (and I would need to test it myself at some point in a real program). Having said that there are a few things I want you to consider:

You need to be able to return why a namespace failed to the client. It is not clear to me if this happens... especially relevant in your test which only checks for the regex version. If this fails the you need to able to return a message that goes something like "failed to connect: does not match a namespace pattern for this server".

Really rethink if you need a newNamespace event. IMHO we don't - the smaller the API surface, the better! The example you cite just above works perfectly well because the dynamicNsp variable can be used to watch for events on that set of namespaces. The connect event yields the namespace anyway.

Finally make sure that disconnect on a namespace is isolated to that - it does not disconnect everything. This was a bug I had to fix after realizing it the hard way i.e. in a failing implementation.

[darrachequesne]

@CxRes thanks for your response!

You need to be able to return why a namespace failed to the client. It is not clear to me if this happens... especially relevant in your test which only checks for the regex version. If this fails the you need to able to return a message that goes something like "failed to connect: does not match a namespace pattern for this server".

What should happen when there are several dynamic namespaces? Example:

io.of(/^\/dynamic-\d+$/).on('connect', /* ... */);
io.of(/^\/another-\w+$/).on('connect', /* ... */);
io.of((nsp, query, next) => next(new Error('not implemented yet'))).on('connect', /* ... */);

In that case, which error message should we send to the client?

Really rethink if you need a newNamespace event. IMHO we don't - the smaller the API surface, the better! The example you cite just above works perfectly well because the dynamicNsp variable can be used to watch for events on that set of namespaces. The connect event yields the namespace anyway.

In my example above, the dynamicNsp is a "dummy" namespace. In that implementation (#3195), the client actually joins three namespaces: the default /, the dummy called /_0 and finally the dynamic /dynamic-101:

io.of(/^\/dynamic-\d+$/).on('connect', (socket) => {
  // socket.nsp.name = '/_0'
});

io.on('newNamespace', (nsp) => {
  nsp.on('connect', (socket) => {
    // socket.nsp.name = '/dynamic-101'
  });
});

So in that case I think the newNamespace event is needed, right? That also allows to keep track of the namespaces.
Another possible implementation would be:

• io.of(/^\/dynamic-\d+$/) still returns a dummy namespace
• the dummy namespace forwards the 'connect' events to the actual namespace
• the client only joins / and /dynamic-101

So that we have:

io.of(/^\/dynamic-\d+$/).on('connect', (socket) => {
  // socket.nsp.name = '/dynamic-101'
});

But in that case, what should io.of(/^\/dynamic-\d+$/).emit() do? Delegate to the underlying namespaces (i.e. calling io('/dynamic-101').emit(), io('/dynamic-102').emit(), ...)?

Hope I'm clear enough!

Edit: I updated #3195, could you please take a look?

[CxRes]

@darrachequesne I am beginning to understand where our thought processes differ. You come from a perspective of socket.io coder and I am coming from a user who is not even well versed in the API but desire something to work in a certain way. It would actually have been useful to thrash it out on a board over coffee rather than such an async verbose medium.

My understanding was that the user would define the of() in function/regex form only once. Anything you do with multiple definitions can be done with one and in this case a failure to connect because of a namespace rejection is always well defined. My use case is to watch a arbitrary folder in a directory tree - here I would like to know, for example, if I could not connect because the folder did not exist or access was denied. This would only be sensible with if dynamic namespace was defined once.

I suspect your objection will be that in static case we do not define server = io.of(['static1', 'static2', ...]) and socketio emit events to each namespace separately. But in my opinion, this is how it will always be when you move to dynamic namespaces. Any dynamic definition anyway simulates io.of(['static1', 'static2', ...]) and it is in this context server API must be thought of (again why it should have been fixed before a v2.0).

This also dovetails into the emit objection. io.of(fn) will be the same as io(), if it is defined only once (In terms of your new code / is the only parent namespace). I am not sure though why you write io('/dynamic-101').emit() Isn't that on the client side and would be read on the socket object in the server (which are well defined per namespace)? I am not even sure as to why you want a separate server object for each namespace is always desirable.

[adamreisnz]

Between all the different github issues, it is really hard to figure out what has been merged/implemented and what hasn't, in terms of dynamic namespaces. The documentation doesn't mention any of the above solutions at all. I've spent some time implementing what I thought was released in 2.1.0 (useNamespaceValidator) only to find out that useNamespaceValidator is not a function.

It'd be great if someone could update the documentation with what the current state and functionality of dynamic namespaces is.

Edit: for anyone else finding this, looks like the regex approach works and is implemented:

   io
      .of(/^\/[a-z0-9-.]+$/)
      .on('connect', socket => {
        console.log('DYNAMIC:', socket.nsp.name);
      });

[CxRes]

I am not using socket.io anymore, in part because the change that was landed in 2.x was not consistent with this proposal. In particular, the maintainers refused to implement any way for clients to learn the reason for rejection of a namespace (and the code base had changed significantly between 1.x and 2.x for non-maintainers to PR a fix without considerable study)!

To see what has been actually implemented in master see PR #3195!

[adamreisnz]

What alternative are you using now, if I may ask?

…

On Wed, 16 Sep 2020, 22:47 CxRes, ***@***.***> wrote: I am not using socket.io anymore, in part because the change that was landed in 2.x was not consistent with this proposal. In particular, the maintainers refused to implement any way for clients to learn the reason for rejection of a namespace (and the code base had changed significantly between 1.x and 2.x for non-maintainers to PR a fix without considerable study)! To see what has been actually implemented in master see PR #3195 <#3195>! — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1921 (comment)>, or unsubscribe </~https://github.com/notifications/unsubscribe-auth/AADXYQTCIRDIFK4OM5ZC74DSGCJUDANCNFSM4AZKYABA> .

[CxRes]

What alternative are you using now, if I may ask?

I have switched to ordinary websockets for now (its not ideal though). I wish I could be of more help!

[darrachequesne]

@adamreisnz I agree, there could be more examples about dynamic namespaces in the documentation.

Dynamic namespaces are mentioned in the Server API and in the Namespaces page though.

[adamreisnz]

Yes, I noticed it on the Server API page eventually, but missed it on the Namespaces pages. It only has a brief mention on the namespaces page, but I reckon it probably deserves its own section under Namespaces.