guzba / mummy

An HTTP and WebSocket server for Nim that returns to the ancient ways of threads.
MIT License
281 stars 11 forks source link

Can't start server in another thread because serve internally accesses global allocated memory #65

Closed MCRusher closed 1 year ago

MCRusher commented 1 year ago

http10 and http11 are declared as global let variables in mummy.nim which prevents calling serve in another thread using createThread since anything that uses them are non-gcsafe

simple example program that doesn't compile:

import mummy, mummy/routers

var router: Router

router.get("/") do(r: Request):
    r.respond(200, @[("Content-Type", "text/plain")], "Hello, World!")

var server = newServer(router)

var thrd: Thread[Server]
thrd.createThread(param=server, tp=proc(s: Server) =
    s.serve(Port(80), "127.0.0.1")
)

discard stdin.readLine() # server should end after host presses enter

I took a crack at fixing it and this is what I did to get it (seemingly) working

const
  http10 = "HTTP/1.0"
  http11 = "HTTP/1.1"
...
if dataEntry.recvBuf[space2 + 1..^1].startsWith(http11):
  dataEntry.requestState.httpVersion = Http11
elif dataEntry.recvBuf[space2 + 1..^1].startsWith(http10):
  dataEntry.requestState.httpVersion = Http10
else:
  return true # Unsupported HTTP version, close the connection

for comparison, it used to be:

let
  http10 = "HTTP/1.0"
  http11 = "HTTP/1.1"
...
if equalMem(
  dataEntry.recvBuf[space2 + 1].addr,
  http11[0].unsafeAddr,
  8
):
  dataEntry.requestState.httpVersion = Http11
elif equalMem(
  dataEntry.recvBuf[space2 + 1].addr,
  http10[0].unsafeAddr,
  8
):
  dataEntry.requestState.httpVersion = Http10
else:
  return true # Unsupported HTTP version, close the connection
guzba commented 1 year ago

Thanks for reporting the issue. I do not want to address this the way your code has since it results in allocations taking those slices. An easier fix is to just add {.gcsafe.}: block to tell the compiler to go away, reading is perfectly safe. It also keeps the non-allocation. I'll sit down and add this soon.

MCRusher commented 1 year ago

yeah it'd be nice if startsWith took an offset to avoid it, I was hoping the compiler would elide the allocation somehow.

alright, thanks a lot

guzba commented 1 year ago

I learned recently Nim's std/strutils does have continuesWith https://nim-lang.org/docs/strutils.html#continuesWith%2Cstring%2Cstring%2CNatural but I still prefer a very explicit comparison.

A smarter Nim compiler could in theory skip the alloc for the slices in theory, and it has been discussed but it doesn't work now, and even if it could, it's then "how do I prove this is not getting copied" since recvBuf could be arbitrarily large.

It's not so easy since all it would take is a .cstring or something like that anywhere in the procs to trigger a copy (though maybe the compiler could delay it until then or something?).

Anyway, it may be low level but equalMem is extremely simple and predictable so I'm in.

I have merged the fix into #head but will not be tagging a release for a bit yet as I'm ironing out the HTTP multipart handling before tagging it. Will comment here again when it is tagged.

guzba commented 1 year ago

This is tagged in release 0.3.0 https://github.com/guzba/mummy/releases/tag/0.3.0