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 promise is not properly awaited #51

Closed
martin-cll opened this issue Jan 26, 2023 · 1 comment
Closed

Websocket promise is not properly awaited #51

martin-cll opened this issue Jan 26, 2023 · 1 comment
Assignees

Comments

@martin-cll
Copy link
Contributor

martin-cll commented Jan 26, 2023

Hi team, I think I may have found a bug with how we're awaiting the websocket connection. In the websocket transport's streamHandler method, it awaits the promise returned by establishWsConnection. This promise is resolved in the ws conn's open callback. In theory, this await should block until the connection is successfully established and the open callback is called. In practice, I believe Node is early exiting from the streamHandler async function and the background executor proceeds to its next iteration loop. See: nodejs/node#22088.

I discovered this after adding many logger statements in and around establishWsConnection to see what is happening and no log lines after establishWsConnection was being emitted, but there were no exceptions. Here is a snippet of those logs:

DEBUG [2023-01-25 23:54:25.172]: [] [WebSocketTransport] No established connection and new subscriptions available, connecting to WS
DEBUG [2023-01-25 23:54:25.172]: [] [WebSocketTransport] this.providerDataStreamEstablished=1674690865172
DEBUG [2023-01-25 23:54:25.172]: [] [WebSocketTransport] build conn handlers
DEBUG [2023-01-25 23:54:25.172]: [] [WebSocketTransport] wsConnection.readyState=0
DEBUG [2023-01-25 23:54:25.172]: [] [WebSocketTransport] added event listeners
DEBUG [2023-01-25 23:54:25.172]: [] [StreamingTransport] Setting local state to subscription set value
TRACE [2023-01-25 23:54:25.172]: [] [BackgroundExecutor] Finished background execute for endpoint "price", calling it again in 1ms...

These logs show that:

  1. We set this.providerDataStreamEstablished right before the call to establishWsConnection.
  2. Inside establishWsConnection's promise, we build the connection handlers, create the conn object (with an initial state of 0 i.e. connecting), and add the event listeners.
  3. a. At this point we expect to block until the conn is created with a corresponding log from the open callback (Opened websocket connection. (event type ${event.type})) but no such log appears.
    b. Additionally we would also expect log lines from streamHandler after establishWsConnection such as Sending subs/unsubs if there are any or Websocket handler complete, sleeping for but they also don't appear.
  4. Instead the next log is from outside streamHandler, from the backgroundHandler function that calls streamHandler.

This issue blocks me from updating to any version that includes #16. That PR moves the sleep from the background executor to inside the transport. However, because of early exiting, we will never await the sleep at the end of the websocket transport's streamHandler and as a result the execution loop runs every 1ms. Additionally, this.connectionOpenedAt is never set properly so the next iteration will attempt to close the (not opened yet) connection causing more errors. It's possible this issue existed from the initial commit, but the fact that the bg executor slept for 5s somehow masked its effects.

Note: to be clear, I don't encounter this issue in integration tests because I assume the mock websocket conn & server is fast enough to not be a problem, but I do encounter this issue when running the EA connecting to the DP directly.

@alejoberardino
Copy link
Contributor

Not tracking here

@alejoberardino alejoberardino closed this as not planned Won't fix, can't repro, duplicate, stale Feb 13, 2023
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