marblejs / marble

Marble.js - functional reactive Node.js framework for building server-side applications, based on TypeScript and RxJS.
https://marblejs.com
MIT License
2.15k stars 69 forks source link

Websocket timeout causes process to crash #345

Closed matthewpflueger closed 2 years ago

matthewpflueger commented 3 years ago

Describe the bug Following the documentation, I setup a basic hello world http and websocket server (code below). Running the code works fine but after some time of inactivity a TimeoutError is thrown which kills the entire process. This seems like unexpected behavior. Is that correct? Anyway to prevent/catch the TimeoutError?

To Reproduce Compile and run the code below. Send a hello message then walk away. After some time the following exception occurs killing the process:

$> tsc lib/server/http.ts      
$> node lib/server/http.js 
λ - 60355 - 2021-07-17 23:45:17 - websockets [Context] - Registered: "LoggerToken"
λ - 60355 - 2021-07-17 23:45:17 - http [Context] - Registered: "HttpServerClient"
λ - 60355 - 2021-07-17 23:45:17 - http [Context] - Registered: "HttpRequestBus"                                                                                                                       
λ - 60355 - 2021-07-17 23:45:17 - http [Context] - Registered: "LoggerToken"                                                                                                                          
λ - 60355 - 2021-07-17 23:45:17 - http [Context] - Registered: "HttpServerEventStream"                                                                                                                
λ - 60355 - 2021-07-17 23:45:17 - http [Context] - Registered: "WebSocketServer"                                                                                                                      
λ - 60355 - 2021-07-17 23:45:17 - http [Router] - Effect mapped: / GET                                                                                                                                
λ - 60355 - 2021-07-17 23:45:17 - http [Router] - Effect mapped: /(.*) *                                                                                                                              
λ - 60355 - 2021-07-17 23:45:17 - http [Server] - Server running @ http://127.0.0.1:8007/ 🚀                                                 
λ - 60355 - 2021-07-17 23:45:37 - websockets [Server] - Connected incoming client "c554d454-858c-4d4c-aadd-70f0fa7dee87" (::ffff:127.0.0.1)
New client connection
λ - 60355 - 2021-07-17 23:46:10 - websockets [EVENT_IN] - HELLO, id: c554d454-858c-4d4c-aadd-70f0fa7dee87
λ - 60355 - 2021-07-17 23:46:10 - websockets [EVENT_OUT] - HELLO, id: c554d454-858c-4d4c-aadd-70f0fa7dee87 and sent to "::ffff:127.0.0.1"
λ - 60355 - 2021-07-18 00:58:05 - websockets [Server] - Closed connection form client "c554d454-858c-4d4c-aadd-70f0fa7dee87" (::ffff:127.0.0.1)
λ - 60355 - 2021-07-18 00:58:05 - websockets [ServerListener] - OutgoingEvent stream completes
λ - 60355 - 2021-07-18 00:58:05 - websockets [ServerListener] - OutgoingEvent stream completes
λ - 60355 - 2021-07-18 00:58:05 - websockets [ServerListener] - OutgoingEvent stream completes

node_modules/rxjs/dist/cjs/internal/util/reportUnhandledError.js:13
            throw err;
            ^                                                                                      
Error
    at _super (node_modules/rxjs/dist/cjs/internal/util/createErrorClass.js:7:26)
    at new TimeoutErrorImpl (node_modules/rxjs/dist/cjs/internal/operators/timeout.js:14:9)
    at timeoutErrorFactory (node_modules/rxjs/dist/cjs/internal/operators/timeout.js:60:11)
    at AsyncAction.<anonymous> (node_modules/rxjs/dist/cjs/internal/operators/timeout.js:37:34)
    at AsyncAction.<anonymous> (node_modules/rxjs/dist/cjs/internal/util/caughtSchedule.js:8:21)
    at AsyncAction._execute node_modules/rxjs/dist/cjs/internal/scheduler/AsyncAction.js:76:18)
    at AsyncAction.execute (node_modules/rxjs/dist/cjs/internal/scheduler/AsyncAction.js:64:26)
    at AsyncScheduler.flush (node_modules/rxjs/dist/cjs/internal/scheduler/AsyncScheduler.js:39:33)
    at listOnTimeout (internal/timers.js:554:17)                                                   
    at processTimers (internal/timers.js:497:7) {
  message: 'Timeout has occurred',
  info: { meta: null, lastValue: null, seen: 371 }
}

The code:

import {
  bindEagerlyTo,
  createContextToken,
  createServer,
  httpListener,
  HttpServerEffect,
  matchEvent,
  r,
  ServerEvent,
} from '@marblejs/core'
import {
  createWebSocketServer,
  mapToServer,
  webSocketListener,
  WebSocketServerConnection,
  WsEffect,
  WsServerEffect,
} from '@marblejs/websockets'
import { IO } from 'fp-ts/lib/IO'
import { merge } from 'rxjs'
import { mapTo, tap } from 'rxjs/operators'

const WebSocketServerToken = createContextToken<WebSocketServerConnection>('WebSocketServer')

export const hello$: WsEffect = event$ =>
  event$.pipe(matchEvent('HELLO'), mapTo({ type: 'HELLO', payload: 'Hello, world!' }))

const connection: WsServerEffect = (event, _ctx) =>
  event.pipe(
    matchEvent(ServerEvent.connection),
    tap(() => console.log('New client connection')),
  )

const webSocketServer = createWebSocketServer({
  event$: (...args) => merge(connection(...args)),
  listener: webSocketListener({
    effects: [hello$],
  }),
})

const upgrade$: HttpServerEffect = (event$, ctx) =>
  event$.pipe(
    matchEvent(ServerEvent.upgrade),
    mapToServer({
      path: '/api',
      server: ctx.ask(WebSocketServerToken),
    }),
  )

const api$ = r.pipe(
  r.matchPath('/'),
  r.matchType('GET'),
  r.useEffect(req$ => req$.pipe(mapTo({ body: 'Hello, world!' }))),
)

const server = createServer({
  dependencies: [bindEagerlyTo(WebSocketServerToken)(async () => await (await webSocketServer)())],
  event$: (...args) => merge(upgrade$(...args)),
  listener: httpListener({
    effects: [api$],
  }),
  port: 8007,
})

const main: IO<void> = async () => await (await server)()

main()

Expected behavior A TimeoutError should not kill the process and/or there should be a way to catch and handle this error.

Desktop (please complete the following information):

Additional context

matthewpflueger commented 3 years ago

So, being a noob to rxjs I didn't know about the GlobalConfig that has an onUnhandledError property that you can set with a function to handle the error (see below). Handling the error prevents the process from crashing. Anyway, I still don't understand what is timing out but the error seems to not be indicative of bigger problems as once handled the process stays up and keeps working as expected. I will leave this issue open until the library author has a chance to give any insight...

import { config } from 'rxjs'
config.onUnhandledError = console.error
JozefFlakus commented 3 years ago

@matthewpflueger what client you use for connecting with websocket server? Does it handle heartbeat? Maybe there is a I'm not sure if the error comes from handleClientBrokenConnection function but it is worth to check. I'll try to validate it.

BTW. this is the only place in the websockets module where the timeout operator is explicitly used 🤔

matthewpflueger commented 3 years ago

@JozefFlakus I am using WebSocket King to connect and no, I don't think it is handling the heartbeat. If that is the cause then this is likely a non-issue as in production the client will handle the heartbeat.

FYI switched to marblejs@next in order to have "official" rxjs 7 support and so far so good!

JozefFlakus commented 2 years ago

@matthewpflueger did you notice any mentioned problems so far with the nightly build? 🤔

matthewpflueger commented 2 years ago

@matthewpflueger did you notice any mentioned problems so far with the nightly build? 🤔

Nope! So far I haven't run into this issue again...

JozefFlakus commented 2 years ago

Ok, I'll keep it open till the official release of v4.0.

jan-kacina commented 2 years ago

Is there any workaround/solution for this issue in v3? I'm experiencing the same issue and I think I won't be able to upgrade to v4 before production release...

Just a quick observation, maybe it helps to analyze the issue - I'm using websockets for front-end - back-end communication and this error occurs almost every time when the front-end application runs in iOS browser (regardless it's Safari, Chrome, ....) When using browser on Android or Windows the error has occurred once per several hours.

jan-kacina commented 2 years ago

I've little bit investigated it... In my case the problem is caused by return throwError(() => error).

I suppose catchError is intended to catch error thrown by timeout operator several lines above and terminate the client. Is it really necessary to throw an error at that place? When I changed return throwError(() => error) to, e.g. return of(error) outgoing client is correctly logged and (mainly) the application process is not killed.

Reproduction info:

JozefFlakus commented 2 years ago

@jan-kacina version 3.0 is available under this branch. Could you open PR with a fix?

JozefFlakus commented 2 years ago

@jan-kacina fix is here. Will release it under v3.5.2 then propagate it to v4.0.1

JozefFlakus commented 2 years ago

v3.5.2 and v4.0.1 released