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

Add websocket support to redbean #967

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

Conversation

wingdeans
Copy link

@wingdeans wingdeans commented Nov 27, 2023

I would like websocket support in redbean #439.
I think it can be done most easily with the following Lua API, which builds on the coroutine support implemented for streaming:

function OnHttpRequest()
    if GetPath() == '/ws' then
        UpgradeWS() -- Set status/headers for handshake
        -- Additional headers can be set after UpgradeWS and before yield
        coroutine.yield() -- Conduct handshake

        local fds = {[GetClientFd()] = unix.POLLIN}
        while true do
            unix.poll(fds)
            print(ReadWS()) -- Do stuff with packet

            Write("Received data") -- Write data
            coroutine.yield() -- Flush
        end
    end
end

This PR implements the handshake and framing, so that the above example functions.

TODO:

  • Fragmentation
  • Ping
  • Text frame
  • Tests (via autobahn-testsuite)
  • Redbean tests
  • SetHeader('Upgrade': 'websocket')

Autobahn Test Results:

["FAILED",1]
["INFORMATIONAL",3]
["NON-STRICT",4]
["OK",293]
["UNIMPLEMENTED",216]

Please feel free to give criticism of the API or code; there are definitely improvements to be made.

@pkulchenko
Copy link
Collaborator

pkulchenko commented Nov 27, 2023

@wingdeans, I like the simplicity of it; thank you for the patch! Is there a basic client implementation to check the server against?

I like the idea of integrating with coroutine.yield to interact with the client.

I'll have to think about the API; I wonder if UpgradeWS implementation can be hidden behind Sec-WebSocket-Key header assignment, thus eliminating the need for this call.

@wingdeans
Copy link
Author

wingdeans commented Nov 27, 2023

For the initial commit I used my browser to test

var sock = new WebSocket("ws://localhost:8080/ws")
sock.onmessage = console.log
sock.send("hello, world")

I'm now using autobahn-testsuite with the following config and redbean echo server:

{
   "outdir": "./reports/servers",
   "servers": [
      {
         "url": "ws://127.0.0.1:8080"
      }
   ],
   "cases": ["*"],
   "exclude-cases": [],
   "exclude-agent-cases": {}
}
-- Updated Dec 28
ProgramMaxPayloadSize(20 * 1024 * 1024) -- 20Ki, for certain autobahn tests

function OnHttpRequest()
    UpgradeWS()
    coroutine.yield()

    local fds = {[GetClientFd()] = unix.POLLIN}
    while true do
        unix.poll(fds)
        local s, flags = ReadWS()
        if flags & 0xF == 0x8 then
            return
        elseif flags & 0xF == 0x1 then
            Write(s)
            SetWSFlags(1)
            coroutine.yield()
        elseif flags & 0xF == 0x2 then
            Write(s)
            SetWSFlags(2)
            coroutine.yield()
        end
    end
end

@pkulchenko
Copy link
Collaborator

@wingdeans, thank you for the updates! Do you have any feedback on my earlier comments?

@coderofsalvation
Copy link

exciting!

local fds = {[GetClientFd()] = unix.POLLIN}

this is setting a dynamic key while declaring a table? no idea lua could do that ❤️

@wingdeans
Copy link
Author

I'll have to think about the API; I wonder if UpgradeWS implementation can be hidden behind Sec-WebSocket-Key header assignment, thus eliminating the need for this call.

Sec-WebSocket-Key is a client-sent header, but upgrading the connection on Upgrade: websocket could work. Something like:

SetStatus(101)
SetHeader('Upgrade', 'websocket')
-- Now in a WS
Write('packet part 1')
Write('packet part 2')
coroutine.yield()

@pkulchenko
Copy link
Collaborator

upgrading the connection on Upgrade: websocket could work.

I like that! I agree that it should work and can't think of any reason why someone would not want that.

We may need to be careful though, as another SetStatus call can reset the headers, so this may need to be taken into account in SetStatus to set iswebsocket back to false.

@wingdeans
Copy link
Author

All essential tests are passing. Current API considerations:

  1. I think SetHeader('Upgrade', 'websocket') needs to raise an exception if the client didn't send a Sec-WebSocket-Key. Otherwise, the yield would need to raise.
  2. Two functions are being introduced: ReadWS and SetWSFlags. ReadWS returns a string and the kind of message received. Right now the kind is an int:
local s, flags = ReadWS()
-- 0 = continuation, 1 = text, 2 = binary, 9 = ping, 10 = pong

I plan on adding constants for these.

SetWSFlags just sets if server should send a binary or text ws message.

I'm planning on moving everything to a ws module, like

local s, kind = ws.Read()
if kind == ws.Text then
    ws.Set(ws.Text)
elseif kind == ws.Bin then
    ws.Set(ws.Bin)
end

Feedback is appreciated.

@pkulchenko
Copy link
Collaborator

I like the API (including moving it into its own module). I was thinking about adding ws.Write (for consistency with ws.Read), but maybe we can leverage it and trigger SetHeader('Upgrade', 'websocket') from ws.Write if cpm.wstype is not set, which can also check for Sec-WebSocket-Key? This way the user only needs to be mostly concerned with ws.Read and ws.Write (and coroutine.yield()).

@wingdeans
Copy link
Author

Feels nice enough

ws.Write(nil) -- Upgrade without sending response
coroutine.yield()

local fds = {[GetClientFd()] = unix.POLLIN}
while true do
    unix.poll(fds)
    local s, t = ws.Read()
    if t == ws.CLOSE then
        return
    elseif t == ws.TEXT then
        ws.Write(s, ws.TEXT)
        coroutine.yield()
    elseif t == ws.BIN then
        ws.Write(s, ws.BIN)
        coroutine.yield()
    end
end

Just cleanup/tests/docs left

@pkulchenko
Copy link
Collaborator

Nice! Would it make sense to imply TEXT by default for ws.Write?

Also, what's this value https://github.com/jart/cosmopolitan/pull/967/files#diff-a937cc5818818785e079eab14ff76fa5db080fe9bd902d1ab21aa546aa79c7d1R5089 and why is it hardcoded?

@wingdeans
Copy link
Author

Right now it defaults to not changing the mode. That GUID is defined by the RFC.

For this header field, the server has to take the value (as present
in the header field, e.g., the base64-encoded [RFC4648] version minus
any leading and trailing whitespace) and concatenate this with the
Globally Unique Identifier (GUID, [RFC4122]) "258EAFA5-E914-47DA-
95CA-C5AB0DC85B11" in string form, which is unlikely to be used by
network endpoints that do not understand the WebSocket Protocol. A
SHA-1 hash (160 bits) [FIPS.180-3], base64-encoded (see Section 4 of
[RFC4648]), of this concatenation is then returned in the server's
handshake.

@pkulchenko
Copy link
Collaborator

Right now it defaults to not changing the mode.

Got it; it changes the current output and all subsequent ones.

@pkulchenko
Copy link
Collaborator

@wingdeans, bool haskey can be removed, as it's no longer used. I can add the docs; do you plan to add a couple of tests?

@jart jart force-pushed the master branch 2 times, most recently from faa6285 to 2c4b887 Compare July 25, 2024 08:23
@jart jart force-pushed the master branch 2 times, most recently from 3048620 to 6245732 Compare December 22, 2024 06:13
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

Successfully merging this pull request may close these issues.

3 participants