Sample server crashes on socket close

[moigagoo]

websocket.nim version: 0.3.3
Nim version: 0.18.0, devel

1. Create a server and client from the docs samples:

   • http://niv.github.io/websocket.nim/docs/0.3.3/websocket/client.html#example
   • http://niv.github.io/websocket.nim/docs/0.3.3/websocket/server.html#example

2. Compile and run the server and client.

3. Observe as they exchange data.

4. Stop the client.

Expected behavior: the server continues running
Actual behavior: the server craches with this trace:

multicart_backend.nim(33) multicart_backend
asyncdispatch.nim(1654)  waitFor
asyncdispatch.nim(1514)  poll
asyncdispatch.nim(353)   runOnce
asyncdispatch.nim(189)   processPendingCallbacks
asyncmacro.nim(36)       recvFrame_continue
shared.nim(156)          recvFrameIter
[[reraised from:
multicart_backend.nim(33) multicart_backend
asyncdispatch.nim(1654)  waitFor
asyncdispatch.nim(1514)  poll
asyncdispatch.nim(353)   runOnce
asyncdispatch.nim(189)   processPendingCallbacks
asyncmacro.nim(36)       readData_continue
shared.nim(243)          readDataIter
asyncfutures.nim(302)    read
]]
[[reraised from:
multicart_backend.nim(33) multicart_backend
asyncdispatch.nim(1654)  waitFor
asyncdispatch.nim(1514)  poll
asyncdispatch.nim(353)   runOnce
asyncdispatch.nim(189)   processPendingCallbacks
asyncmacro.nim(36)       cb_continue
multicart_backend.nim(16) cbIter
asyncfutures.nim(302)    read
]]
[[reraised from:
multicart_backend.nim(33) multicart_backend
asyncdispatch.nim(1654)  waitFor
asyncdispatch.nim(1514)  poll
asyncdispatch.nim(353)   runOnce
asyncdispatch.nim(189)   processPendingCallbacks
asyncmacro.nim(36)       processRequest_continue
asynchttpserver.nim(259) processRequestIter
asyncfutures.nim(302)    read
]]
[[reraised from:
multicart_backend.nim(33) multicart_backend
asyncdispatch.nim(1654)  waitFor
asyncdispatch.nim(1514)  poll
asyncdispatch.nim(353)   runOnce
asyncdispatch.nim(189)   processPendingCallbacks
asyncmacro.nim(36)       processClient_continue
asynchttpserver.nim(288) processClientIter
asyncfutures.nim(302)    read
]]
[[reraised from:
multicart_backend.nim(33) multicart_backend
asyncdispatch.nim(1654)  waitFor
asyncdispatch.nim(1514)  poll
asyncdispatch.nim(353)   runOnce
asyncdispatch.nim(189)   processPendingCallbacks
asyncfutures.nim(348)    :anonymous
]]
Error: unhandled exception: socket closed
Async traceback:
  multicart_backend.nim(33) multicart_backend
  asyncdispatch.nim(1654)   waitFor
  asyncdispatch.nim(1514)   poll
    ## Processes asynchronous completion events
  asyncdispatch.nim(353)    runOnce
  asyncdispatch.nim(189)    processPendingCallbacks
    ## Executes pending callbacks
  asyncmacro.nim(36)        recvFrame_continue
    ## Resumes an async procedure
  shared.nim(156)           recvFrameIter
  #[
    multicart_backend.nim(33) multicart_backend
    asyncdispatch.nim(1654)   waitFor
    asyncdispatch.nim(1514)   poll
      ## Processes asynchronous completion events
    asyncdispatch.nim(353)    runOnce
    asyncdispatch.nim(189)    processPendingCallbacks
      ## Executes pending callbacks
    asyncmacro.nim(36)        readData_continue
      ## Resumes an async procedure
    shared.nim(243)           readDataIter
    asyncfutures.nim(302)     read
  ]#
  #[
    multicart_backend.nim(33) multicart_backend
    asyncdispatch.nim(1654)   waitFor
    asyncdispatch.nim(1514)   poll
      ## Processes asynchronous completion events
    asyncdispatch.nim(353)    runOnce
    asyncdispatch.nim(189)    processPendingCallbacks
      ## Executes pending callbacks
    asyncmacro.nim(36)        cb_continue
      ## Resumes an async procedure
    multicart_backend.nim(16) cbIter
    asyncfutures.nim(302)     read
  ]#
  #[
    multicart_backend.nim(33) multicart_backend
    asyncdispatch.nim(1654)   waitFor
    asyncdispatch.nim(1514)   poll
      ## Processes asynchronous completion events
    asyncdispatch.nim(353)    runOnce
    asyncdispatch.nim(189)    processPendingCallbacks
      ## Executes pending callbacks
    asyncmacro.nim(36)        processRequest_continue
      ## Resumes an async procedure
    asynchttpserver.nim(259)  processRequestIter
    asyncfutures.nim(302)     read
  ]#
  #[
    multicart_backend.nim(33) multicart_backend
    asyncdispatch.nim(1654)   waitFor
    asyncdispatch.nim(1514)   poll
      ## Processes asynchronous completion events
    asyncdispatch.nim(353)    runOnce
    asyncdispatch.nim(189)    processPendingCallbacks
      ## Executes pending callbacks
    asyncmacro.nim(36)        processClient_continue
      ## Resumes an async procedure
    asynchttpserver.nim(288)  processClientIter
    asyncfutures.nim(302)     read
  ]#
Exception message: socket closed
Exception type: [IOError]

[metagn]

How did you "stop the client"? With Ctrl+C? Did you do ws.sock.close()? The way to stop it that would have worked in this case is ws.close() on the client so the server receives the Close opcode.

The problem in this case is how the library treats closed sockets that haven't given a Close opcode which is currently with an IOError. To get around it you'd do this:

try:
  let (opcode, data) = await ws.readData()
  echo "(opcode: ", opcode, ", data: ", data, ")"
except IOError, ProtocolError:
  ws.sock.close()
  echo "closing websocket because of error: ", getCurrentExceptionMsg()

Or this:

let (opcode, data) =
  try:
    await ws.readData()
  except:
    ws.sock.close()
    (Opcode.Close, "")
echo "(opcode: ", opcode, ", data: ", data, ")"

But the question remains if the library should either:

• not bother raising an exception and just close errored sockets (would be bad for the too large payload error)
• return a library-only opcode named Error and have the error be specified in the data string
• return a different opcode for each error like SocketClosed, PayloadTooBig and so on
• introduce different exception types for each error

If we're gonna keep exceptions which we probably are, I prefer the last one as using IOError everywhere is probably not a good idea

[moigagoo]

Yes, I stop the client with Ctrl+C, which is not a valid socket close event. I think, however, the server should be tolerant to that, so a fix in the sample server code will be enough to get rid of the confusion.

As a workaround, I wrapped my code in a try block, but your solutions are cleaner, I especially like the second one.

On a general note, maybe a switch from nil- and exception-based flow to using options will make the lib interface better?

[hiteshjasani]

Parts of your stack trace look similar to a problem I was having in #52 and put a fix in #55. You might want to get HEAD and see if you still have this issue.