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

not GC-safe #6

Open
zevv opened this issue Dec 11, 2018 · 14 comments

Comments

@zevv
Copy link

commented Dec 11, 2018

Some of my Nim projects use asyncHttpServer, and I'm always nagged by the requirement for GC safe HTTP handlers, even when not using threads but only async.

Curious to see how this was handled in Jester I turned to your example code, just to find you have the same problem here :)

The example code from chapter 7:

../../../../../home/ico/external/Nim/lib/pure/asyncmacro.nim(267, 31) Warning: 'matchIter' is not GC-safe as it accesses 'db' which is a global using GC'ed memory [GcUnsafe2]

Compiling with --threads:on turns this warning into an error, even.

What would be the best way to handle this?

Thanks,

@dom96

This comment has been minimized.

Copy link
Owner

commented Dec 11, 2018

Good question. @Araq, please advise.

@Araq

This comment has been minimized.

Copy link

commented Dec 12, 2018

Well usually we say "use a thread variable".

@zevv

This comment has been minimized.

Copy link
Author

commented Dec 12, 2018

@Araq

This comment has been minimized.

Copy link

commented Dec 12, 2018

But that's exactly what asyncHttpServer does, it is single-threaded. I don't understand you.

@zevv

This comment has been minimized.

Copy link
Author

commented Dec 12, 2018

That's my point: if it is single threaded, why does it require a {.gcsafe.} callback function?

@dom96

This comment has been minimized.

Copy link
Owner

commented Dec 12, 2018

AFAIK the reason is that if the callback is not marked gcsafe then as soon as you use asynchttpserver with --threads:on you will start getting errors because the compiler won't be able to prove that the callback is gc safe.

@Araq

This comment has been minimized.

Copy link

commented Dec 13, 2018

Exactly. I consider this question answered.

@zevv

This comment has been minimized.

Copy link
Author

commented Dec 13, 2018

Very sorry to be a nuisance, and I'm taking the risk to be regarded as stupid, but I'm afraid I
still don't understand.

Async is nothing else then callback functions, so I do not understand at all where the problem with threads come in. Even with threads enabled: as long as the event queue is polled() from the proper thread, how can there ever arise any problems with shared data?

As discussed on #nim with dom, here is a naive patch I tried on the asyncHttpServer lib. This removes the GC-safe requirement on the async handler callback, which allows me to write async HTTP servers with threads enabled and no .treadvar. or .gcsafe. pragmas anywhere, compiling just fine with threads:

diff --git a/lib/pure/asynchttpserver.nim b/lib/pure/asynchttpserver.nim
index d27c2fb..f31019b 100644
--- a/lib/pure/asynchttpserver.nim
+++ b/lib/pure/asynchttpserver.nim
@@ -147,7 +147,7 @@ proc processRequest(server: AsyncHttpServer, req: FutureVar[Request],
                     client: AsyncSocket,
                     address: string, lineFut: FutureVar[string],
                     callback: proc (request: Request):
-                      Future[void] {.closure, gcsafe.}) {.async.} =
+                      Future[void] {.closure.}) {.async.} =
 
   # Alias `request` to `req.mget()` so we don't have to write `mget` everywhere.
   template request(): Request =
@@ -277,7 +277,7 @@ proc processRequest(server: AsyncHttpServer, req: FutureVar[Request],
 
 proc processClient(server: AsyncHttpServer, client: AsyncSocket, address: string,
                    callback: proc (request: Request):
-                      Future[void] {.closure, gcsafe.}) {.async.} =
+                      Future[void] {.closure.}) {.async.} =
   var request = newFutureVar[Request]("asynchttpserver.processClient")
   request.mget().url = initUri()
   request.mget().headers = newHttpHeaders()
@@ -288,7 +288,7 @@ proc processClient(server: AsyncHttpServer, client: AsyncSocket, address: string
     await processRequest(server, request, client, address, lineFut, callback)
 
 proc serve*(server: AsyncHttpServer, port: Port,
-            callback: proc (request: Request): Future[void] {.closure,gcsafe.},
+            callback: proc (request: Request): Future[void] {.closure.},
             address = "") {.async.} =
   ## Starts the process of listening for incoming HTTP connections on the
   ## specified address and port.

I have tried to follow the history of the .gcsafe. pragmas in Git, and I found that the first occurence was added by Araq in 2014 with the commit message "asynchttpserver compiles again" in
nim-lang/Nim@62e454f#diff-3133183ae7c444b928e3e590f4b1d217.

My only guess is that the whole .gcsafe. propagation is ment to protect the user from polling the event loop from the wrong thread, could that be the original idea?

@zevv

This comment has been minimized.

Copy link
Author

commented Dec 13, 2018

Ok, answering my own last question for the sake of archiving and properly closing this issue: the whole point of the .gcsafe. seems to be to protect the user against the case where the event loop is polled from the "wrong" thread, as the compiler can not infer this.

The price to pay is that the user has to perform precautions to tell the compiler "hush, I know what I'm doing" by adding .gcsafe. and .threadvar. pragmas.

@Araq

This comment has been minimized.

Copy link

commented Dec 13, 2018

.gcafe is inferred for you, .threadvar cannot.

@yglukhov

This comment has been minimized.

Copy link

commented Dec 18, 2018

seems to be to protect the user against the case where the event loop is polled from the "wrong" thread

I don't think that's the case. If that happens everything will blow up regardless gcsafety.

I suspect the gcsafety restriction on asyncHttpServer callbacks comes from the times when future completion and gcsafety was not yet sorted (e.g. nim-lang/Nim#5738). Now that overall async-gcsafety restriction is removed (nim-lang/Nim#6059) I tend to think that asyncHttpServer gcsafety requirement can be relaxed as well.

@zevv

This comment has been minimized.

Copy link
Author

commented Dec 18, 2018

That was my guess as well, I was able to remove the gcsafe pragmas and have working code without warnings or errors. But beging a Nim newbe and having bothered Dom and Araq too many times about this (I still don't understand, can you explain again) I didn't feel like pushing it any further.

@zevv

This comment has been minimized.

Copy link
Author

commented Dec 18, 2018

Also: if the gcsafe restriction really applies to asyncHttpServer, should it not apply to all async? I can now simply create my own async http server without the gcsafe restriction.

@jamiesonbecker

This comment has been minimized.

Copy link

commented Apr 19, 2019

It does seem to be a good question.

/~/.choosenim/toolchains/nim-0.19.4/lib/pure/asyncmacro.nim(273, 31) Warning: 'matchIter' is not GC-safe as it accesses 'db' which is a global using GC'ed memory [GcUnsafe2]

This is using a default jester and httpbeast, which as you know prefers multi-threading.

But, it also seems like database access (especially sqlite) will block the (single) async thread (and it's not operated in a threadpool, correct?)

So, what is the most efficient way of writing a jester-backed server that writes to disk or database and blocks the event loop?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.