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 startup callback to settings #89

Merged
merged 6 commits into from
Aug 7, 2023
Merged

Add startup callback to settings #89

merged 6 commits into from
Aug 7, 2023

Conversation

ThomasTJdev
Copy link
Contributor

This PR implements a startup proc option to the Settings* allowing to run a custom proc when each thread is initialized (when the eventLoop is called).

The PR includes a test case where two global {.threadvar.} variables are initialized at thread-start. Without the startup proc the user would need to check whether the variables are initiated in each thread (on every call) or otherwise create them.

Copy link
Owner

@dom96 dom96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI failing

Copy link
Owner

@dom96 dom96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the startup proc the user would need to check whether the variables are initiated in each thread (on every call) or otherwise create them.

Is that really so slow?

Comment on lines 563 to 572
addExitProc(proc() =
for thr in threads:
when compiles(pthread_cancel(thr.sys)):
discard pthread_cancel(thr.sys)
if not isNil(thr.core):
when defined(gcDestructors):
c_free(thr.core)
else:
deallocShared(thr.core)
)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for fix error SIGSEGV: Illegal storage access. (Attempt to read from nil?)
https://github.com/dom96/httpbeast/actions/runs/5778815517/job/15660432786#step:4:34

@bung87
Copy link
Contributor

bung87 commented Aug 6, 2023

so it's the first time run test against to nim v2, even I just want add feature, I have to write extra code make test green

@bung87
Copy link
Contributor

bung87 commented Aug 6, 2023

and second time I look into tester's startServer logic, I really hope it can change to start random port and retrieve that port then make request to that address, so no kill and start on same port.

@ThomasTJdev
Copy link
Contributor Author

Without the startup proc the user would need to check whether the variables are initiated in each thread (on every call) or otherwise create them.

Is that really so slow?

For the code, no - it's just checking a bool, but there's still the overhead of doing the check everywhere and the waiting for the first connection before initializing database connections, activate custom thread monitoring, parsing init-files.

The code example below outlines how the current situation is - if you need a database dbConn or just a global threadvar. And the problem "scales" up to e.g. Jester, where the same checks is needed.

var isDbInit {.threadvar.}: bool
var db {.threadvar.}: DbConn

var isVarsInit {.threadvar.}: bool
var versionNr  {.threadvar.}: string

proc initDb() =
  db = openDbConn()

proc initVars() =
  let dict = parseCfg("..")
  versionNr = dict("..")

proc onRequest(req: Request): Future[void] =
  if req.httpMethod == some(HttpGet):
    case req.path.get()
    of "/":
      # We need the DB to respond, but we need to check whether it is initialized
      # or otherwise for the connection.
      if not isDbInit:
        initDb()
      let dbValue = getValue(db, sql"...")
      req.send("name:$#." % [dbValue])
    else:
      # We need a global variable, but we need to check whether it is initialized
      # or otherwise initialize them.
      if not isVarsInit:
        initVars()
      req.send("version:$#." % [versionNr])

run(onRequest, initSettings())

@dom96
Copy link
Owner

dom96 commented Aug 7, 2023

fwiw this should be a feature offered by Nim's threading

The Nim 2.0 fix seems like a workaround to a bug in Nim as well.

But this is simple enough that I'll just merge it to unblock you since you use httpbeast in production I presume.

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