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

WebSocket transport broken for dedicated worker scope #569

Closed
1 of 2 tasks
SupremeTechnopriest opened this issue May 17, 2017 · 8 comments
Closed
1 of 2 tasks

WebSocket transport broken for dedicated worker scope #569

SupremeTechnopriest opened this issue May 17, 2017 · 8 comments

Comments

@SupremeTechnopriest
Copy link

Note: for support questions, please use one of these channels: stackoverflow or slack

You want to:

  • report a bug
  • request a feature

Current behaviour

WebSocket transport does not work in web worker.

Steps to reproduce (if the current behaviour is a bug)

Inside of a webworker, start socket.io with websocket transport only:

io(`localhost:8080`, {
    transports: [ 'websocket' ],
    upgrade: false
});

Expected behaviour

I expect to be able to connect to a websocket from a webworker.

Setup

Other information (e.g. stacktraces, related issues, suggestions how to fix)

I believe these are the offending lines https://github.com/socketio/engine.io-client/blob/master/lib/transports/websocket.js#L11-L28

window and global are not defined inside of the dedicated worker scope. WebSocket however is. Presuming a server environment from the lack of a window object is not safe for modern applications.

I suggest something like the following:

var BrowserWebSocket = global.WebSocket || global.MozWebSocket;
var WorkerWebSocket = WebSocket;
var NodeWebSocket;
if (!WorkerWebSocket && typeof window === 'undefined') {
  try {
    NodeWebSocket = require('ws');
  } catch (e) { }
}

/**
 * Get either the `WebSocket` or `MozWebSocket` globals
 * in the browser, the dedicated `WebSocket` scope from a worker
 * or try to resolve WebSocket-compatible
 * interface exposed by `ws` for Node-like environment.
 */

var WebSocket = BrowserWebSocket || WorkerWebSocket;
if (!WebSocket && typeof window === 'undefined') {
  WebSocket = NodeWebSocket;
}

I haven't tested this yet, but will probably be the next thing I do.

@darrachequesne
Copy link
Member

What's weird is that there's a test for that: https://github.com/socketio/engine.io-client/blob/3.1.0/test/connection.js#L63

Which seems to pass in saucelabs:
image

Is there any error thrown?

@SupremeTechnopriest
Copy link
Author

no errors thrown, but I cant enable debug from the worker scope (no localstorage)

@SupremeTechnopriest
Copy link
Author

SupremeTechnopriest commented May 18, 2017

Looks like the test https://github.com/socketio/engine.io-client/blob/3.1.0/test/support/public/worker.js is just testing general connection functionality. If you start socket.io with default options it does connect, but it will never upgrade to websockets. If you explicitly specify the transport in your test it will most likely fail. I really prefer websockets for this task. XHR polling is also extremely noisy in the network tab.

@SupremeTechnopriest
Copy link
Author

SupremeTechnopriest commented May 18, 2017

I haven't used engine.io raw before, but I'm assuming socket.io is just sugar on top of engine.io. The transport logic actually lives here in engine.io

@SupremeTechnopriest
Copy link
Author

SupremeTechnopriest commented May 18, 2017

So I wrote a test case for websocket transports, but zuul doesn't want to run locally. 😞 Any chance you would be available to hop on slack and work through this @darrachequesne ? I'll be working on it until It's done. The last thing I need before I can ship this product.

edit: zuul probably related to this defunctzombie/zuul#314

@SupremeTechnopriest
Copy link
Author

Thanks for fixing the local tests. I've been able to reject my hypothesis. Things seem to be working fine as far as engine.io-client is concerned... The bug is else where. My hunt ends here for now. I will find another way to solve my problem.

@darrachequesne
Copy link
Member

That may be linked to the debug package, there was a few issues related to browser detection lately: https://github.com/visionmedia/debug/blob/master/CHANGELOG.md

@SupremeTechnopriest
Copy link
Author

Yes. Probably that. I had to pull debug out of my worker completely.

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

No branches or pull requests

2 participants