Allows passing by reference through spawn* if is a shared type

[ErnyTech]

It is often useful to pass a mutable by reference type to the spawned
thread, the need to have to use a raw pointer before this patch made
the syntax boring and not in line with the rest of the DLang style
(use pointer as little as possible).

With this patch it will be possible to write code like this:

import std.concurrency : spawn;
import core.atomic : atomicOp;
import core.thread : thread_joinAll;

static void f1(ref shared(int) number)
{
  atomicOp!"+="(number, 1);
}

shared(int) number = 10;
spawn(&f1, number);
thread_joinAll();
assert(number == 11);

Instead of the boring:

import std.concurrency : spawn;
import core.atomic : atomicOp;
import core.thread : thread_joinAll;

static void f1(ref shared(int)* number)
{
  atomicOp!"+="(*number, 1);
}

shared(int) number = 10;
spawn(&f1, &number);
thread_joinAll();
assert(number == 11);

[dlang-bot]

Thanks for your pull request and interest in making D better, @ErnyTech! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#7124"

[ErnyTech]

no one wants to review this?

[ErnyTech]

CC @wilzbach

[ErnyTech]

@atilaneves Now do you think it can be approved?

[ErnyTech]

circleci has failed for dmd/src/dmd/dmsc.d:110: undefined reference to out_config_debug(bool, bool, bool, bool, bool, bool, bool) is really weird

[PetarKirov]

If you rebase to master the error should go away. I fixed it in #7160.

[ErnyTech]

Ok now circleci is green, thanks!

[PetarKirov]

I think the pointer-based syntax has the advantage of signifying to the reader that the spawned function will access by reference the arguments.

With concurrency one should be extra careful about the objects' lifetime.
GC-allocated class objects have relatively easier to manage lifetime, so passing them implicitly by reference is not a big problem. However stack allocated objects have much shorter lifetime which makes it especially easy to get into undefined behavior by escaping their address. Of course technically references have no more problems than pointers, but for human readers it is also easier to miss when a piece of code shares the address of an object.

Consider the following:

void main()
{
    int var;
    f();
}

void f()
{
    import std.concurrency : spawn;
    // run asynchronously, without immediately awaiting it to finish
    shared(int) number = 10;
    spawn(&g, number); // passed by value before
                        // after: accesses an expired stack frame from another thread
}

void g(ref shared(int) number)
{
    import core.atomic : atomicOp;
    atomicOp!"+="(number, 1);
}

[ErnyTech]

I do not believe that a reader has particular difficulties in understanding what is happening when he reads "ref" in the parameters

[thewilsonator]

This seems to result in a consistent failure in excel-d.

ut.wrap.wrap.wrapAsync:
--
  | tests/ut/wrap/wrap.d:30 - Expected: 6.400000
  | tests/ut/wrap/wrap.d:30 -      Got: nan

ut.wrap.module_.wrapModuleFunctionStr async double -> double:
--
  | tests/ut/wrap/module_.d:620 - Expected: 6.000000
  | tests/ut/wrap/module_.d:620 -      Got: nan

@atilaneves

[ErnyTech]

How about making a spawnRef function to make everyone happy and avoid any kind of code breaks?

[ErnyTech]

I believe there is a logical error in excel-d, this function (https://github.com/kaleidicassociates/excel-d/blob/91034ca039979f945828820e2eafd817963a7a1e/source/xlld/wrap/wrap.d#L544) contains ref in the parameters but we know that before this PR could not be passed by ref with spawn, in my opinion this is what causes the test to fail

However, this keeps me thinking that maybe spawnRef is the best solution, at least you don't risk breaking code

[PetarKirov]

I'd be okay with having a spawnRef that took all of its arguments by ref. No need for auto ref as now there's -preview=rvaluerefparam to account for rvalues.

[ErnyTech]

Alternative PR: #7167

[atilaneves]

I think that spawn should work with functions that take their arguments by ref. It's a limitation that doesn't make any sense to me at the moment. I don't think that explicitly adding a spawnRef function makes much sense.

[ErnyTech]

@atilaneves any advice to solve the problem with excel-d?

[PetarKirov]

@atilaneves what about the example I provided in #7124 (comment)? We can't allow such brutally unsafe things, while other languages such as Rust focus on concurrency safety, in addition to memory safety.

[atilaneves]

@atilaneves any advice to solve the problem with excel-d?

@ErnyTech I haven't had time to look into it. I'll try tomorrow but if not then I'm on holiday for 3 weeks.

[atilaneves]

@atilaneves what about the example I provided in #7124 (comment)? We can't allow such brutally unsafe things, while other languages such as Rust focus on concurrency safety, in addition to memory safety.

@ZombineDev How is the ref version less safe than the pointer one? I understand the argument about how ref makes it invisible to the caller that the argument may be modified; it's been made before in the Google C++ style guide and in Rust (I remain unconvinced, possibly because I make everything and its sibling const and have never written a bug due to it). That however seems to be an odd argument to make in a language that already has ref.

Lifetime problems need to be dealt with the same way regardless if an argument is ref T or T*; your example is equally buggy when using a pointer. That needs to be fixed (although spawn et al are all @System now anyway).

[RazvanN7]

Filed: https://issues.dlang.org/show_bug.cgi?id=21848

Closing.