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

Figure out a way to deal with Send/Sync/thread safety in languages where this matters #533

Open
Tracked by #534
Manishearth opened this issue Jul 9, 2024 · 5 comments

Comments

@Manishearth
Copy link
Contributor

Manishearth commented Jul 9, 2024

The current set of "primary" Diplomat backends (C++, C, JS, Dart) do not need to care about thread safety: C and C++ are places where the "norm" is that you need to manually document/figure out thread safety, and JS/Dart don't let objects jump to other threads.

However, this isn't ideal for C/++ and this won't work for Java/Kotlin/etc. References to opaque types transferred to other threads will be able to deal with mutable state concurrently.

I'm not sure there is a one-size-fits-all solution here. The general thrust of any solution will be:

  • Types can be threadsafe or not threadsafe (this could be opt-in or opt-out)
  • Backends can choose to (potentially configurably) implement synchronization guards on top of not-threadsafe types
  • Backends can choose to document not-threadsafe types as such
  • Types marked threadsafe are enforced to be Sync and Send by diplomat-macro
  • Types marked threadsafe are disallowed from being accessed as &mut by HIR lowering
  • Backends can choose to document threadsafe types as such

It occurred to me to return such types as Arc<T> instead of Box<T> but I don't actually think that buys us anything that just "enforcing Send and Sync" doesn't.

Probably needs #225

@jcrist1
Copy link
Contributor

jcrist1 commented Jul 11, 2024

@Manishearth When I was first playing with jvm ffi stuff, it also occurred to me that Box<T> should be enough for thread safe access, because the GC should take care of cleanup, so we don't need reference counting, as long as we have T: Send + Sync as you said. I managed to get mutable access with Box<Mutex<T>> without leaks (which is a pattern I never thought I'd see), but adds some nastiness, as now we can implicitly get deadlocks if we're not careful. If we went down this path it would be nice if there could be some kind of guaranteed lock order for methods accepting multiple mutable references. It might be more reasonable to keep synchronization in the backend language. May be relevant for #225

@Manishearth
Copy link
Contributor Author

It might be more reasonable to keep synchronization in the backend language

As in, the backend manages synchronization? How would this look for, say, Java or Kotlin?

@Manishearth
Copy link
Contributor Author

Also for the purposes of this discussion it may be worth assuming #225 as a prerequisite

@jcrist1
Copy link
Contributor

jcrist1 commented Jul 12, 2024

If we had the opaque_mut attribute then we could automatically add a locking interface to the class, then we could add a random id that's used for global lock order
like

interface Lockable {
    final ReadWriteLock lock = new ReentrantReadWriteLock();
    final UUID lockId = UUID.randomUUID();
}.

public class MyString implements Lockable {
...
}

Then when we actually want to call them we order the locks with the UUIDs

public class SomeClass {
    public ReturnType someMethod(MyString myStr1, MyString myStr2) {
        var locks = new Lockable[]{myStr, myStr2}; // Here we could probably combine with a custom accessor to preferentially choose how to lock it (read or write depending on the reference access type, below I just defaulted to write)
        Arrays.sort(locks, new Comparator<Lockable>() {
                    @Override
                    public int compare(Lockable lockable1, Lockable lockable2 ) {
                        return lockable1.lockId.compareTo(lockable2.lockId);
                    }
                });
        Arrays.stream(locks).forEach(myString -> myString.lock.writeLock().lock());
        // Regular conversions and method call
        ...
        Arrays.stream(locks).forEach(myString-> myString.lock.writeLock().unlock()); // this can be added to cleanups if any parameters of a method are `opaque_mut`
        return returnVal;
    }
}
...

@Manishearth
Copy link
Contributor Author

Makes sense. opaque_mut is not super hard to add, it would just be an additional field on Opaque and we'd need a validation pass after HIR lowering.

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

No branches or pull requests

2 participants