moigagoo / norm

A Nim ORM for SQLite and Postgres
https://norm.nim.town
MIT License
378 stars 34 forks source link

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

Closed d0rksh closed 1 year ago

d0rksh commented 1 year ago

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'

PhilippMDoerner commented 1 year ago

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
d0rksh commented 1 year ago

thank you its working now :)

PhilippMDoerner commented 1 year ago

@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?

moigagoo commented 1 year ago

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.

PhilippMDoerner commented 1 year ago

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)?

moigagoo commented 1 year ago

@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.

ajusa commented 1 year ago

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.

moigagoo commented 1 year ago

@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.

PhilippMDoerner commented 1 year ago

@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.

moigagoo commented 1 year ago

@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.

PhilippMDoerner commented 1 year ago

@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.

ire4ever1190 commented 1 year ago

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

PhilippMDoerner commented 1 year ago

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

moigagoo commented 1 year ago

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

PhilippMDoerner commented 1 year ago

Agreed