Skip to content
This repository has been archived by the owner on Sep 3, 2022. It is now read-only.

Race condition when setting up a shim for socket.io #2069

Closed
ojarjur opened this issue Aug 28, 2018 · 1 comment · Fixed by #2070
Closed

Race condition when setting up a shim for socket.io #2069

ojarjur opened this issue Aug 28, 2018 · 1 comment · Fixed by #2070
Assignees
Labels

Comments

@ojarjur
Copy link
Contributor

ojarjur commented Aug 28, 2018

There is a race condition bug in the way we setup socket.io when we are replacing the native WebSocket implementation (which is necessary when connecting from Cloud Shell).

The issue is that our websocket.js code, which replaces the native window.WebSocket with our shim, runs after the main.min.js code, which creates the Kernel object.

The Kernel code does not directly call window.WebSocket. Instead, it copies the value of window.WebSocket at the time it was created into a field of the object, and then calls the value of that field.

This means that if we replace the window.WebSocket value before the Kernel object gets created, then our shim works as expected. However, if that replacement does not happen until the Kernel object is created, then the shim will not work.

We have two options to fix this:

  1. Ensure the websocket.js code runs before the main.min.js code
    or
  2. Make the websocket.js code modify the field in the Kernel object.

I'm not sure if the second option is even possible, so I think the first is by far the best choice.

@ojarjur ojarjur added the bug label Aug 28, 2018
@ojarjur ojarjur self-assigned this Aug 28, 2018
@ojarjur
Copy link
Contributor Author

ojarjur commented Aug 28, 2018

The way this bug will manifest is a pop-up error message saying Connection failed and A connection to the notebook server could not be established. The notebook will continue trying to reconnect. Check your network connection or notebook server configuration. when opening up the Datalab UI using the WebPreview feature of Cloud Shell.

If this occurs, it should be a rare occurrence, and refreshing the page will likely fix it.

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

Successfully merging a pull request may close this issue.

1 participant