HttpBeast random segfault on orc and threads > 1

[dirtboll]

Description

Original issue was posted here dom96/httpbeast#80.

This bug arises when adding new logging handler in httpbeast application that's using orc and threads more than 1. Below is the minimal code that could reproduce the issue (taken from httpbeast example with only addition being addHandler).

# hello.nim
import options, asyncdispatch, logging

import httpbeast

proc onRequest(req: Request): Future[void] =
  if req.httpMethod == some(HttpGet):
    case req.path.get()
    of "/":
      req.send("Hello World")
    else:
      req.send(Http404)

if logging.getHandlers().len == 0:
  addHandler(logging.newConsoleLogger())

run(onRequest, httpbeast.initSettings(numThreads=2))

I've tried replicating this by implementing httpbeast Settings object flow, which is being passed between 2 threads that I thought was the root cause, but it didn't give segfault. So far, I'm only able to replicate the bug when adding new ConsoleLogger in httpbeast applications, which also affects jester as well.

Nim Version

Latest nightlies for linux_x64 downloaded here https://github.com/nim-lang/nightlies/releases/tag/latest-version-2-0

Nim Compiler Version 1.9.1 [Linux: amd64]
Compiled at 2023-02-19
Copyright (c) 2006-2022 by Andreas Rumpf

git hash: 414f36980f17ee80a48b2f38e035b7e94f2112bd
active boot switches: -d:release

Also tested on Nim release 1.6.8

Current Output

Starting 2 threads
Listening on port 8080
SIGSEGV: Illegal storage access. (Attempt to read from nil?)
Segmentation fault (core dumped)
Error: execution of an external program failed: '/home/ubuntu/hello'

Expected Output

Starting 2 threads
Listening on port 8080

Possible Solution

I found that I can solve the issue by commenting this block of code. Other workaround that I did was modifying Settings.loggers to ref seq[Logger], which also removes the random segfault.

https://github.com/dom96/httpbeast/blob/0f7b07f00e4c90eceef90e27da6e997a111407cc/src/httpbeast.nim#L336-L339

Additional Information

• httpbeast version 0.4.1
• Tested on GCP VM with Ubuntu 22.04.1 LTS

[PMunch]

I also just ran into this today. Unfortunately it makes Jester not work on latest devel which is quite bad.

[ThomasTJdev]

Also in the process of upgrading my code to 1.6.12 before the 2.0 release, but all code using jester and httpbeast with threads will fail.

[ringabout]

reduced

import selectors, net, nativesockets, os, posix

from osproc import countProcessors

type
  Data = object

type
  Ref = ref object

  Settings* = object
    port*: Port
    bindAddr*: string
    domain*: Domain
    numThreads*: int
    loggers*: seq[Ref]

  HttpBeastDefect* = ref object of Defect


proc initSettings*(port: Port = Port(8080),
                   bindAddr: string = "",
                   numThreads: int = 0,
                   domain = Domain.AF_INET,
                  ): Settings =
  Settings(
    port: port,
    bindAddr: bindAddr,
    domain: domain,
    numThreads: numThreads,
    loggers: @[Ref()],
  )


proc eventLoop(
  params: tuple[settings: Settings, isMainThread: bool]
) =
  let (settings, isMainThread) = params

  echo settings.domain

  let selector = newSelector[Data]()

proc run*(settings: Settings) =
  when compileOption("threads"):
    let numThreads =
      if settings.numThreads == 0: countProcessors()
      else: settings.numThreads
  else:
    let numThreads = 1

  echo("Starting ", numThreads, " threads")
  if numThreads > 1:
    when compileOption("threads"):
      var threads = newSeq[Thread[(Settings, bool)]](numThreads - 1)
      for t in threads.mitems():
        createThread[(Settings, bool)](
          t, eventLoop, (settings, false)
        )
    else:
      assert false
  echo("Listening on port ", settings.port) # This line is used in the tester to signal readiness.
  eventLoop((settings, true))

run(initSettings(numThreads=2))

[beef331]

Assuming that's a faithful reproduction this is not a Nim problem. The following program ran with ./programName fail shows the same problem as the above code, it's resolved with a joinThreads to ensure that the reference is not deallocated before the threads terminate.

import selectors, net, nativesockets, os, posix

from osproc import countProcessors

type
  Data = object

type
  Ref = ref object

  Settings* = object
    val: Ref

proc eventLoop(
  settings: Settings
) =
  let settings = settings # Copy value so destructor kicks in
  echo cast[int](settings.val)

  let selector = newSelector[Data]()

proc run*(settings: Settings) =
  for x in 0 .. 100:
    let numThreads = 2
    if paramCount() == 1 and paramStr(1) == "fail":
      if numThreads > 1:
        var threads = newSeq[Thread[Settings]](numThreads - 1)
        for t in threads.mitems():
          createThread[Settings](
            t, eventLoop, settings
          )
      echo "main loop start"
      eventLoop(settings)
    else:
      var threads: seq[Thread[Settings]]
      if numThreads > 1:
        threads = newSeq[Thread[Settings]](numThreads - 1)
        for t in threads.mitems():
          createThread[Settings](
            t, eventLoop, settings
          )
      echo "main loop start"
      eventLoop(settings)
      joinThreads(threads)

run(Settings(val: Ref()))

[ringabout]

It seems that my reduce is not good enough.

[ringabout]

I don't think it's right way for httpbeast to work with threads in ORC. It should implement a startup event so that Logger can be initialized each thread without passing refs between threads.

It works for my fork => https://github.com/ringabout/httpx/blob/a3a2a9f65ac6fb3a3153e5379d561d99079e0402/src/httpx.nim#L529

# hello.nim
import options, asyncdispatch, logging

import httpx

proc onRequest(req: Request): Future[void] =
  logging.debug "fine"
  if req.httpMethod == some(HttpGet):
    case req.path.get()
    of "/":
      req.send("Hello World")
    else:
      req.send(Http404)

proc startup() =
  if logging.getHandlers().len == 0:
    addHandler(logging.newConsoleLogger())

addHandler(logging.newConsoleLogger())
run(onRequest, initSettings(numThreads=2, startup=startup))

[ThomasTJdev]

I don't think it's right way for httpbeast to work with threads in ORC. It should implement a startup event so that Logger can be initialized each thread without passing refs between threads.

I'm still able to produre SIGSEGV: Illegal storage access. (Attempt to read from nil?) even when I have commented out all logging in httpbeast.

I have tried to with split screen between httpbeast and httpx and commented out one line at the time but with no helpful outcome :| . I have also tested httpx, but I never got it to fail like httpbeast..

[ringabout]

I'm still able to produre SIGSEGV: Illegal storage access. (Attempt to read from nil?) even when I have commented out all logging in httpbeast.

All of them?

You need to comment addHandler in the eventloop function.

https://github.com/dom96/httpbeast/blob/0f7b07f00e4c90eceef90e27da6e997a111407cc/src/httpbeast.nim#L338

Or https://github.com/dom96/httpbeast/blob/0f7b07f00e4c90eceef90e27da6e997a111407cc/src/httpbeast.nim#L88

[ThomasTJdev]

I'm still able to produre SIGSEGV: Illegal storage access. (Attempt to read from nil?) even when I have commented out all logging in httpbeast.

All of them?

Yes:
git clone [email protected]:dom96/httpbeast.git && cd httpbeast

Settings(
    port: port,
    bindAddr: bindAddr,
    domain: domain,
    numThreads: numThreads,
    # loggers: getHandlers(),
    reusePort: reusePort,
    listenBacklog: listenBacklog,
  )

...

# if not isMainThread:
  #   # We are on a new thread. Re-add the loggers from the main thread.
  #   for logger in settings.loggers:
  #     addHandler(logger)

Test
File: httpbeast/tests/sig.nim
Cmd: nim c -d:dev --mm:orc -r tests/sig.nim

import options, asyncdispatch

import .. / src / httpbeast

proc onRequest(req: Request): Future[void] =
  if req.httpMethod == some(HttpGet):
    case req.path.get()
    of "/":
      req.send("Hello World")
    else:
      req.send(Http404)

block:
  let settings = initSettings()

  run(onRequest, settings)

Output
Start program, quit with ctrl+c, repeat between 8-20 times.

Starting 8 threads
Listening on port 8080
No stack traceback available
SIGSEGV: Illegal storage access. (Attempt to read from nil?)
Error: execution of an external program failed: '/home/user/git/httpbeast/tests/s

[ringabout]

Weird, I will try again later.

[bung87]

I confirmed @beef331 's solution works, and my thought is the issue because httpbeast use eventLoop in main thread keep threads not cancel, when server stoped, the threads suddenly destroyed and related resource not properly free. I push a patch merged seem worked out.