norm icon indicating copy to clipboard operation
norm copied to clipboard

Error: 'matchIter' is not GC-safe as it calls 'insert'

Open d0rksh opened this issue 2 years ago • 5 comments

hi i was trying to create a jester app with SQLite as a database, when i try to compile the code with --threads:on compiler throws a error

code:

post "/api/adddomain":
      var domain = parseJson(request.body)
      var domains =   domain.to(Domain)
      var ins = Monitor(domain: domains.domain ,discard_webhook:domains.hook)
      try:
          dbConn.insert(ins)
          var jsonResp = $(%*{"message": "success","id":ins.id,"domain":domains.domain,"hook":domains.hook})
          resp Http200, jsonResp

var jester1 = initJester()
jester1.serve()

error when i compile with --threads:on sr/lib/nim/pure/asyncmacro.nim(200, 31) Error: 'matchIter' is not GC-safe as it calls 'insert'

d0rksh avatar Aug 18 '22 12:08 d0rksh

I'm not entirely sure if it's on norm to deal with this, but here on how to deal with this for the time being:

d0rksh, all the compiler is seeing is that you're accessing a "global" resource ultimately with a db.exec command. We know that it is fine since the database will have locks implemented and the like, but the compiler doesn't. So we can cast the corresponding expression as gc-safe to tell the compiler that "Yeah, you found something that might not be GC-safe but I've personally figured out that it's fine".. Example: '

proc createEntry*[T: Model](connection: DbConn, entry: var T): T =
      {.cast(gcsafe).}:
          connection.insert(entry)
          result = entry

PhilippMDoerner avatar Aug 19 '22 05:08 PhilippMDoerner

thank you its working now :)

d0rksh avatar Aug 19 '22 06:08 d0rksh

@moigagoo There is a valid point in here though: Should norm's currently non-gcsafe-procs be cast as gcsafe so that the compiler doesn't notice them as gc-unsafe in multi-threaded projects of norm-users? Or is that information that we want the user to have?

PhilippMDoerner avatar Aug 19 '22 06:08 PhilippMDoerner

Should norm's currently non-gcsafe-procs be cast as gcsafe so that the compiler doesn't notice them as gc-unsafe in multi-threaded projects of norm-users?

When using Norm with Prologue, I just mark my controller procs gcsafe explicitly to make the compiler happy, for example:

proc getUpdates*(ctx: Context) {.async, gcsafe.} =

So, there is at least a less hacky way of solving the problem than cast.

moigagoo avatar Aug 23 '22 08:08 moigagoo

I entirely agree that it's an easy problem to solve it's mostly a question on whether to have this convenience factor (of making the corresponding norm procs explicitly gc-safe so users downstream don't have to make their own procs explicitly gcsafe) or not.

Am I interpreting you correctly that you're currently erring on the side of not doing it (which is perfectly valid, I myself have no preference either way, just thought it might be valid to discuss)?

PhilippMDoerner avatar Aug 23 '22 10:08 PhilippMDoerner

@PhilippMDoerner I've just tried adding {.gcsafe.} to insert and that doesn't solve the issue. You still have to mark your Plologue procs as GC-safe to make the compiler happy.

moigagoo avatar Oct 10 '22 07:10 moigagoo

Just ran into this again with https://github.com/guzba/mummy. I think the Norm procs do need to be annotated as well, rather than doing the somewhat unsafe cast gcsafe.

ajusa avatar Dec 11 '22 17:12 ajusa

@ajusa I honestly tried annotating the Norm procs as gcsafe but that didn't help. If you have any idea or better yet a PR I'll be happy to fix that issue in Norm.

moigagoo avatar Dec 11 '22 18:12 moigagoo

@moigagoo Was that just annotating them or was that including a cast to gcsafe? So:

proc insert*[T: Model](dbConn; obj: var T, force = false, conflictPolicy = cpRaise) =
  ... lots of code preparing the SQL statement...
  {.cast(gcsafe).}:
    obj.id = dbConn.insertID(sql qry, row)

The key is to find the actual unsafe code. I know the exec proc is for sure, I don't recall about all the others... though I'm pretty sure if it writes to the DB then it'll have to be cast as gcsafe. I know we talked about it back then, I forgot what we all tried out.

PhilippMDoerner avatar Dec 11 '22 18:12 PhilippMDoerner

@PhilippMDoerner I think I tried both: adding gcsafe pragma and casting exec.

There shouldn't be anything unsafe except for exec I think. Because the rest of the proc is just iterations and string construction.

moigagoo avatar Dec 11 '22 18:12 moigagoo

@PhilippMDoerner I think I tried both: adding gcsafe pragma and casting exec.

There shouldn't be anything unsafe except for exec I think. Because the rest of the proc is just iterations and string construction.

Just did this minimal example:

import norm/[model, sqlite]

let con = open(":memory:", "", "", "")

proc myinsert(x: var Model) {.gcsafe.} =
  con.insert x

This will fail with Error: 'myinsert' is not GC-safe as it calls 'insert'.

HOWEVER when you start casting around (I went to my ~/.nimble/pkgs/norm-2.6.0/ folder) and change sqlte.nim line 169 the call to insertID to be:

  {.cast(gcsafe).}:
    obj.id = dbConn.insertID(sql qry, row)

Then it suddenly compiles.

PhilippMDoerner avatar Dec 11 '22 18:12 PhilippMDoerner

Should be fixed once norm switches to lowdb (See https://github.com/PhilippMDoerner/lowdb/pull/3)

ire4ever1190 avatar Feb 19 '23 23:02 ire4ever1190

Norm has switched officially to lowdb as part of PR https://github.com/moigagoo/norm/pull/186

The only thing missing is a new norm version release

PhilippMDoerner avatar Feb 27 '23 18:02 PhilippMDoerner

@PhilippMDoerner let's link this issue with #186 and close it?

moigagoo avatar Feb 27 '23 18:02 moigagoo

Agreed

PhilippMDoerner avatar Feb 27 '23 19:02 PhilippMDoerner