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

closes #8895 add std/once #16192

Closed
wants to merge 8 commits into from
Closed

closes #8895 add std/once #16192

wants to merge 8 commits into from

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Nov 30, 2020

fix #8895
add thread-safe once to std/once, for ORC.

@ringabout ringabout changed the title close #8895, add std/once closes #8895, add std/once Nov 30, 2020
@ringabout ringabout changed the title closes #8895, add std/once closes #8895 add std/once Nov 30, 2020
lib/std/once.nim Outdated Show resolved Hide resolved
lib/std/once.nim Outdated
@@ -0,0 +1,31 @@
import atomics
import locks
Copy link
Member

@timotheecour timotheecour Nov 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about re-entrant code? do we need a ronce / reentrantOnce (rlocks), or can once be made re-entrant safe without overhead?

lib/std/once.nim Outdated Show resolved Hide resolved
timotheecour added a commit to timotheecour/Nim that referenced this pull request Dec 1, 2020
@ringabout ringabout marked this pull request as ready for review December 10, 2020 13:20
Copy link
Contributor

@dom96 dom96 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of implementing a new module for this, can't we fix system.once instead?

for i in 1 .. 10:
once(block1):
inc count
for i in 0..high(thr):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more idiomatic:

for t in mitems(thr):
  createThread(t, threadFunc)

(and Example code in

## for i in 0..high(thr):
should be updated as well in future work)

ditto below

@timotheecour
Copy link
Member

timotheecour commented Dec 10, 2020

Instead of implementing a new module for this, can't we fix system.once instead?

if it can be done transparently for user code and it doesn't add new dependencies to system.nim, yes, but with the way it's done in this PR at least, this isn't the case as it requires an extra parameter Once so would require users to change their code anyways. So it's better to delegate this to another module and avoid bloating system.nim further.

for i in 0..high(thr):
createThread(thr[i], threadFunc)
joinThreads(thr)
doAssert count == 5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else: static doAssert false
(prevents silent misuse)


when defined(caseA):
block:
var thr: array[0..4, Thread[void]]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more idiomatic:

for i in 0..high(thr):
createThread(thr[i], threadFunc)
joinThreads(thr)
doAssert count == 5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doAssert count == thr.len

@@ -0,0 +1,34 @@
discard """
cmd: "nim c -r --threads:on $options $file"
matrix: "-d:caseA; -d:caseB"
Copy link
Member

@timotheecour timotheecour Dec 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here you actually don't need matrix for that since you can combine both:

block:
  var thr: array[0..4, Thread[void]]
  ...

block:
  var thr: array[0..4, Thread[void]]
  ...

but instead, use matrix for:

matrix: "--threads:off; --threads:on"

and adjust code to work with and without threads with compileOption. There are ways to do it without any code duplication.

@@ -0,0 +1,34 @@
discard """
Copy link
Member

@timotheecour timotheecour Dec 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

js needs to be supported because system.once already works in js (not considering edge case of webworkers)

(and leave a TODO for supporting VM because VM already doesn't work with system.once)

when true:
  var count = 0
  var countCT {.compileTime.} = 0
  proc fn(n: int) =
    once:
      when nimvm:
        countCT.inc
      else:
        count.inc
      echo (n,)
    if n > 1:
      fn(n-1)
  proc main() =
    fn(5)
    fn(4)
    when nimvm:
      echo countCT
      # doAssert countCT == 1 # fails
    else:
      doAssert count == 1
  static: main()
  main()

initOnce(block1)
for i in 1 .. 10:
once(block1):
inc count
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we also need a test where the code in once block raises

lib/std/once.nim Outdated
try:
body
finally:
cond.finished.store(true, moRelease)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering whether we should customize whether cond.finished.store(true, moRelease) happens in a finally block or only after a successful body that didn't raise.

there are use cases for both.
eg:

var db: MyDataBase
proc fn=
  once(Once, retryOnFailure = true):
    db = newMyDataBaseFlaky() # suppose this fails 10% of the times
  # we want to make sure `db` is always initialized here
  db.call() # if `newMyDataBase` raised in another thread, `db` will be nil

without retryOnFailure flag, how would you emulate retryOnFailure in user code calling std/once ?

timotheecour added a commit to timotheecour/Nim that referenced this pull request Dec 11, 2020
@juancarlospaco
Copy link
Collaborator

juancarlospaco commented Mar 13, 2021

Should be named std/onces, maybe it can have more than one once overload, etc.

lib/std/once.nim Outdated
finished: Atomic[bool]
lock: Lock

proc initOnce*(once: var Once) =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be func ?.

@konsumlamm
Copy link
Contributor

Should be named std/onces, maybe it can have more than one once overload, etc.

onces doesn't make much sense, it's not even a word (once is an adverb, not a subject).

@juancarlospaco
Copy link
Collaborator

What word is jsffi, cgi, coro, etc 🤷

@planetis-m
Copy link
Contributor

My gripe with this implementation is that a user needs to init once explicitly. This is unusual and can cause UB if its forgotten. An easy way is using a spinlock More advanced implementations are in rust and abseil

@ringabout ringabout closed this Sep 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

system.once is not thread safe
6 participants