-
Notifications
You must be signed in to change notification settings - Fork 53
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI failing
There was a problem hiding this 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?
src/httpbeast.nim
Outdated
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) | ||
) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
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 |
and second time I look into |
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 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()) |
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. |
This PR implements a
startup
proc option to theSettings*
allowing to run a custom proc when each thread is initialized (when theeventLoop
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.