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

Main thread var tagged not GC-safe #4744

Closed
coffeepots opened this issue Sep 9, 2016 · 5 comments
Closed

Main thread var tagged not GC-safe #4744

coffeepots opened this issue Sep 9, 2016 · 5 comments

Comments

@coffeepots
Copy link
Contributor

coffeepots commented Sep 9, 2016

When compiled with --threads:on

import asyncdispatch, jester

var hello: string

proc test(s: string): string =
  result = s

routes: # error reported here
    get "/":
        resp test(hello)

Compiler reports:
c:\nim\lib\pure\asyncmacro.nim(264, 7) Hint: Processing match as an async proc. [User]
test.nim(8, 1) template/generic instantiation from here
c:\nim\lib\core\macros.nim(333, 70) template/generic instantiation from here
c:\nim\lib\pure\asyncmacro.nim(31, 8) Error: 'cb' is not GC-safe as it accesses 'nameIterVar' which is a global using GC'ed memory

Of course, the error is not in asyncmacro.nim.

@nigredo-tori
Copy link
Contributor

nigredo-tori commented Sep 9, 2016

From the manual:

We call a proc p GC safe when it doesn't access any global variable that contains GC'ed memory (string, seq, ref or a closure...

So, by definition, test is not GC-safe, since it accesses hello, which is a global variable.
I agree that the error message doesn't point straight at the root of the issue; but that is tricky to get right with this amount of macros in the mix (someone correct me if I'm wrong).
As for a solution - you would want to think about the way you want to share your data about threads. Here are a couple possible ways:

  1. Channels. Quite easy to use, allow seamless passing of refs, etc.
  2. Share pointers, not refs. You would have to manage memory - by storing the references indefinitely, by using GC_ref and GC_unref, or by (de)allocating manually. You would also want to provide locking for shared values.

@coffeepots
Copy link
Contributor Author

by definition, test is not GC-safe, since it accesses hello, which is a global variable.

That's fine as a definition, but the code above doesn't use any threading - everything's in the main thread. This means that the same code that runs with --threads:off now doesn't compile with --threads:on, before you've even thought about creating a separate thread.

As for a solution - you would want to think about the way you want to share your data about threads.

This problem actually arose because I wanted to run a separate thread completely isolated from every other thread with no communication at all, just as a background worker.

The 'solution' I found, was to mark all variables declared in the main thread as {.threadvar.}, which seemed terribly unsatisfying and generally wrong.

Putting the "routes:" inside a procedure, which is surely something you'd have to do if you wanted to follow the 'no globals' rule, causes C generation errors:

import asyncdispatch, jester

proc main =
  var hello: string

  proc test(s: string): string =
    result = s

  routes: # error reported here
      get "/":
          resp test(hello)

main()

There error in c generation that starts with:

error: incompatible type for argument 1 of 'serve_265873_3416709870'
serve_265873_3416709870(LOC3, settings0);
^

Attempting to compiling this without --threads:on also causes the c-gen error.

@nigredo-tori
Copy link
Contributor

nigredo-tori commented Sep 11, 2016

Putting the "routes:" inside a procedure ... causes C generation errors

This is the result of a lambda lifting bug: #4088 . My workaround is in Jester's master branch, but not yet in the tagged version, so you would have to

nimble install jester@#head

@coffeepots
Copy link
Contributor Author

coffeepots commented Sep 12, 2016

Installed jester@#head and it does indeed fix running the routes macro inside a proc. Thanks a lot for informing me of this fix. Now I can compile the second code snippet above (calling Jester in a proc), which gets rid of the GC unsafe messages too!

There is still the issue of the compiler throwing GC unsafe errors for module level code when the code is "technically thread safe" (though I got the impression this isn't trivial to exorcise), but placing in a proc is a way better work around than what I was doing before.

@Araq
Copy link
Member

Araq commented Mar 1, 2019

There is now also a {.gcsafe.}: body block to override the compiler's ideas if you know you're in the main thread.

@Araq Araq closed this as completed Mar 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants