Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom namespace to avoid conflict with existing Socket.io implementation? #27

Closed
AussieFlem opened this issue Aug 20, 2016 · 23 comments
Closed

Comments

@AussieFlem
Copy link

I plugged e-s-m into our web application with an existing socket.io component, and whilst the monitor system worked it knocked out our existing functionality with a handshaking problem.

I suspect one socket-io killed the other.

Would allowing the specification of namespace allow it to run alongside an existing socket.io implementation, or is it one socket.io per server?

Alternatively, can it be set to use an existing socket.io setup if it's present, and create it's own if not?

I haven't dug through the source, just thought I'd put it out there in case you knew off the top of your head.

@RafalWilinski
Copy link
Owner

Very interesting case, I'll dig into that topic in the next days. I suppose that may be related to clashing socket.io event names.

Thanks for letting me know!

@AussieFlem
Copy link
Author

No worries - let me know if you need me to test anything.

Error is:

socket.io.js:4026 WebSocket connection to 'ws://localhost:3000/socket.io/?EIO=3&transport=websocket&sid=r-kwtVh_JrVyjoNmAAAB' failed: Connection closed before receiving a handshake response

Note that ESM seems to survive, and my socket.io functionality fails, but I suspect that's just the order of the implementation, so an option to prevent the clash might be handy.

@jabis
Copy link

jabis commented Aug 21, 2016

You need to match your socket.io-client library on the clientside to the ^1.4.* that express-status-monitor uses, as well as refactor your connection.on and session handling code, especially if you're using older socket.io ( I've some servers running socket.io-0.9.* which don't play nice with >1.0.0 after ) I've already lost the upgrade-path I used to update beyond that, but it wasn't that hard to do.

EDIT: Not promising anything timeline-wise (haven't even forked ESM yet) but I could pull it to one of the older socket-servers, and test whether I can downgrade ESM to use previous version / upgrade (documenting changes) an older socket-io server to match 1.4.* in the near(-ish) future

UPDATE: Forked to branch esm-downgrade at https://github.com/jabis/express-status-monitor
I managed to get it running properly on older express, and socket.io < 1.0.0
Added a couple of environment variables to config to comply with older spec, used http.createServer wrapping so both socket.io and app can have listen passthru, next I'll try namespace it out and, integrate it with a running app-server, and we'll see if it conflicts.

screenshot

@jabis
Copy link

jabis commented Aug 21, 2016

Managed to integrate with existing socket.io as long as the version of both server and client is of same series and unique namespace is used. 2 distinct socket.io server services clash.
I'm running our own api stack on top of express using a modified vhosts-routing, so I always expose the root-io as a global and vhosts all attach their own namespaces to it, so it took me about a minute to add the config variables, and include the app.use passing the server/socket.io instance after downgrading.

image

@AussieFlem
Copy link
Author

Wow - nice work. Will check it out ASAP and see if it fixes my issue.

@jabis
Copy link

jabis commented Aug 22, 2016

It needs quite a bit more love (I had to embed the old socket.io client to the html-file which is ugly because I was testing it locally first), but as PoC I think you can find the relevant bits there :) Note this was for version ^0.9.16, haven't the time to tackle upgrading the socket servers yet, as doing client work (regarding sockets, unsurprisingly)

@waltergms
Copy link

Same problem here, all socket.io funcionalities in my app have stoped with Monitor installed.

@jabis
Copy link

jabis commented Aug 26, 2016

@waltergms
if you already use socket.io, remove the engine.io dependencies from package.json, and set it up with your socket.io deps instead. Replace the engine io with socket.io client eio.on('conn.. -> io.of('/status').on('conn where eio = engine.io server and io = your socket.io server - and in client replace eio('... with io('/status'

@omnidan
Copy link

omnidan commented Nov 2, 2016

I'm having the same issue. My socket.io instance also uses ^1.4.*, it still doesn't work. When I remove express-status-monitor, my socket.io instance works fine, otherwise I keep getting Connection closed before receiving a handshake response.

Is there any way to re-use the existing socket.io instance yet?

@jabis
Copy link

jabis commented Nov 3, 2016

@omnidan I managed to use the existing socket.io instance just as I described above - replacing the engine io deps with socket.io ones, and the initializers with a namespace.

@omnidan
Copy link

omnidan commented Nov 3, 2016

@jabis I don't use engine.io though - the only dependency I have in my package.json is socket.io. Also, what do you mean by "replace the initializers with a namespace"?

@jabis
Copy link

jabis commented Nov 4, 2016

@omnidan when you install the express-status-monitor, it has a package.json - there is engine.io and engine.io-client references there (which supercede socket.io in a manner of way) and like I posted earlier instead of using eio.on('connection') use io.of('/status').on('connection') and on the clientside eio('...options...) use io('/status') to override the default namespace (and ofc make sure you reference the eio/io with the actual variable for them required socket.io and socket.io-client's packages. Should be that straightforward :)

@omnidan
Copy link

omnidan commented Nov 4, 2016

@jabis Thanks for the explanation! Unfortunately, that would mean that I have to fork express-status-monitor (changing directly in node_modules won't make it work everywhere). Would be nice if this could get fixed in this repo, or maybe an option to pass an existing socket.io instance.

@jabis
Copy link

jabis commented Nov 4, 2016

@omnidan unfortunately it's not my repo so I can't fix this for you :) Also I don't think @AussieFlem will want to change the dependencies to much larger bundle (eio vs io) and it would basically mean a refactor for componizating the whole piece of software to cater for different audiences, as this basically is just a "middleware" per sé. Not only is it conflicting with prior socket.io and engine io releases I suppose it conflicts with every websocket lib instance there is, so it wouldn't be a minor feat. That's actually why I wrote the downgraded version in the first place :) Maybe @AussieFlem though could namespace the lib to a meaningful name, so that it wouldn't hog the default ('/') namespace though?

@omnidan
Copy link

omnidan commented Nov 4, 2016

@jabis Wouldn't it be possible to allow passing an existing socket.io instance and avoid the dependency altogether?

@jabis
Copy link

jabis commented Nov 4, 2016

@omnidan I don't think that npm at least allows conditional dependencies in that sence. The problem mostly lies in the installation process for starters, and adds up upon possible conflicting socket lib used. I guess it could be made a proper middleware though, but I think there is much to go thru in the codebase to reach that point - my advice would be to fork it and keep on top of the changes :)

@omnidan
Copy link

omnidan commented Nov 4, 2016

@jabis I understand that, what I'm talking about is an option in the api config to pass an existing socket.io instance, you could still fall back to eio if no instance is specified.

@jabis
Copy link

jabis commented Nov 4, 2016

@omnidan I get what you're meaning, but when you actually install it and it processes the dependencies you might end up messing your existing socket instance before using it - I suppose the way you suggest would be the best option though - to make it a proper middleware passing along the server and socket instances to bind to - though it doesn't quite address the main problem here :)

@omnidan
Copy link

omnidan commented Nov 4, 2016

@jabis I don't think installing eio will make any difference - I actually still have express-status-monitor installed (I just don't do app.use(require('express-status-monitor')())) and it works fine. So my proposal would be something like this:

app.use(require('express-status-monitor')({ websocket: existingSocketIoInstance }))

@jabis
Copy link

jabis commented Nov 4, 2016

@omnidan yes that's exactly what I'm proposing - though you're wrong in the first assumption - the installed version of socket lib vs the one used on esm does actually make a difference (believe me I've tried :D ) :)

@omnidan
Copy link

omnidan commented Nov 4, 2016

@jabis It's not making any difference for me, but maybe it does if you're using an older version of socket.io (I'm using the latest version while this package uses 1.4)

@RafalWilinski
Copy link
Owner

app.use(require('express-status-monitor')({ websocket: existingSocketIoInstance })) is great idea. I'll try to implement that asap. Thanks for input guys, I really appreciate it.

@RafalWilinski
Copy link
Owner

I think might be resolved by this upcoming change: https://github.com/RafalWilinski/express-status-monitor/pull/62/files

What I'm doing here is allowing to "hook" express-status-monitor events into existing socket.io instance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants