luvit / lit

Toolkit for developing, sharing, and running luvit/lua programs and libraries.
http://lit.luvit.io/
Apache License 2.0
245 stars 58 forks source link

Cannot reliably close WebSocket / coro-websocket read message is never nil #208

Closed SinisterRectus closed 7 years ago

SinisterRectus commented 7 years ago

In coro-websocket 1.0.0-1, I wrote an empty message to close the socket:

write()

This causes an opcode of 8 to be sent:

  local function write(message)
    if message then
      ...
    else -- send close frame if message is nil/false
      if not closeSent then
        return write({
          opcode = 8,
          payload = ""
        })
      end
    end
    return rawWrite(message)
  end

After this, nil was returned to signal an end of data:

  local function read()
    while true do
      local message = rawRead()
      if not message then
        return cleanup() -- this returns nil to break the loop
      end
...

And the read loop breaks:

for message in read do
  -- handle messages
end
-- handle socket close after loop breaks

Similarly, if the server closed the connection, nil is returned and the loop breaks.


While working with coro-websocket 3.0.0, I noticed that nil is not consistently returned.

If the server closes the connection, I get a libuv error and the process hangs:

../coro-channel.lua:119: ../coro-channel.lua:103: ENOTCONN: socket is not connected

If res.socket is closed or if write() is called, the process appears to end (crash?) after one read loop or less; code after the loop is never executed.

PR #207 would help with a server close, but I'm not sure what to do if the client wants to close the connection.

SinisterRectus commented 7 years ago

I think I've narrowed down the issue to somewhere in coro-wrapper by using the following test:

coroutine.wrap(function()

    local ws = require('coro-websocket')

    local options = ws.parseUrl("wss://gateway.discord.gg")
    options.pathname = '/?v=7&encoding=json'

    local res, read, write = ws.connect(options)

    for msg in read do
        write()
    end

end)()

Looking at decoder at line 71:

The final message in chunk is nil, which causes buffer to become an empty string. When this is decoded, both item and newIndex come back as nil. Because they are both nil, the return item line is never reached, so the while true do loop never breaks. The process somehow manages to exit, though. Adding a special case before decoding seems to fix this, but I'm not sure if it will break anything else.

if buffer == '' then
  return nil
end
local item, newIndex = decode(buffer, index)

Another option would be to add it to the final block:

if item or newIndex or buffer == '' then

Note: If I change write() to res.socket:close(), the issue persists. The loop never breaks, but the process exits. This also happens in 1.0.0-1, though, so it may not be related.

SinisterRectus commented 7 years ago

I also noticed that, according to the code, both buffer and chunk can be nil. I don't know if this is ever possible in practice, but if it does happen, decode will attempt to decode a nil buffer. It looks like the HTTP decoder will handle this, but the WebSocket decoder will error. It may be necessary to extend the buffer check:

if buffer == '' or buffer == nil then

... or you can return earlier when the buffer is cleared.

Let me know which fix you think is appropriate and I can PR it, or just let you fix it.

creationix commented 7 years ago

It looks like your head is more in this than mine. I'm super busy with another project for the next few weeks. Go ahead and PR the one you prefer. I'll try it against lit's unit tests and see if it breaks anything in there (they use coro websockets heavily).

SinisterRectus commented 7 years ago

Addressed with 84fe7b9