nim-in-action-code icon indicating copy to clipboard operation
nim-in-action-code copied to clipboard

not GC-safe

Open zevv opened this issue 6 years ago • 14 comments

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,

zevv avatar Dec 11 '18 21:12 zevv

Good question. @Araq, please advise.

dom96 avatar Dec 11 '18 22:12 dom96

Well usually we say "use a thread variable".

Araq avatar Dec 12 '18 09:12 Araq

Quoting Andreas Rumpf (2018-12-12 10:38:15)

Well usually we say "use a thread variable".

I see :)

My problem is that I'm not able to use asyncHttpServer in single-threaded-async mode without needing "kludges" like thread variables.

The name "asyncHttpServer" implies that it is using the async/future mechanism for handling connections, and is thus thread safe to use. The semantics are different from any of the other async<net|socket|dispatch> mechanisms, which is kind of confusing.

I do understand that there is an intention to have it serve on different threads in the future. What about keeping asyncHttpServer purely async/future based so it can be used without precautions in a single thread, and adding a new thing like threadedHttpServer for the case where there are really threads handling the connections?

-- :wq ^X^Cy^K^X^C^C^C^C

zevv avatar Dec 12 '18 09:12 zevv

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

Araq avatar Dec 12 '18 11:12 Araq

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

zevv avatar Dec 12 '18 11:12 zevv

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.

dom96 avatar Dec 12 '18 22:12 dom96

Exactly. I consider this question answered.

Araq avatar Dec 13 '18 08:12 Araq

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 https://github.com/nim-lang/Nim/commit/62e454f41beb5a742dbb8137198629fdc1f1153a#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 avatar Dec 13 '18 12:12 zevv

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.

zevv avatar Dec 13 '18 12:12 zevv

.gcafe is inferred for you, .threadvar cannot.

Araq avatar Dec 13 '18 13:12 Araq

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. https://github.com/nim-lang/Nim/issues/5738). Now that overall async-gcsafety restriction is removed (https://github.com/nim-lang/Nim/pull/6059) I tend to think that asyncHttpServer gcsafety requirement can be relaxed as well.

yglukhov avatar Dec 18 '18 20:12 yglukhov

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 avatar Dec 18 '18 20:12 zevv

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.

zevv avatar Dec 18 '18 20:12 zevv

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?

perpetual-hydrofoil avatar Apr 19 '19 03:04 perpetual-hydrofoil