idris-lang / Idris2

A purely functional programming language with first class types
https://idris-lang.org/
Other
2.53k stars 378 forks source link

[ base ] Add atomically function #3380

Closed Matthew-Mosior closed 2 months ago

Matthew-Mosior commented 2 months ago

This PR adds a new function, atomically to Data.IORef (only for the chez backend).

This function atomically runs its argument according to the provided mutex.

It can for instance be used to modify the contents of an IORef ref with a function f in a safe way in a multithreaded program by using atomically lock (modifyIORef ref f) provided that other threads also rely on the same lock to modify ref.

Credit to @stefan-hoeck for providing code to test this addition in chez003.

Thanks to @gallais for the generalized atomically implementation.

stefan-hoeck commented 2 months ago

One more thought on this: I think in your implementation of atomicModifyIORef you are trying to make sure that the mutex version is only used on Scheme backends. I'm afraid, this won't work, because the compiler has to generate code for every possible branch in your pattern match (but maybe I'm mistaken, and this is indeed how such tests should be performed). In order to verify this, you should add a test that tries compiling and running some code with atomicModifyIORef on Node.js.

Again, there are several possibilities to approach this if the current version does not work:

Matthew-Mosior commented 2 months ago

One more thought on this: I think in your implementation of atomicModifyIORef you are trying to make sure that the mutex version is only used on Scheme backends. I'm afraid, this won't work, because the compiler has to generate code for every possible branch in your pattern match (but maybe I'm mistaken, and this is indeed how such tests should be performed). In order to verify this, you should add a test that tries compiling and running some code with atomicModifyIORef on Node.js.

Again, there are several possibilities to approach this if the current version does not work:

* Let the decision if `atomicModifyIORef` should be used or not be made in client code.

* Write dummy implementations of `prim__makeMutex` and friends for the JavaScript backends. You could, for instance, just return 0 or `undefined` for `prim__makeMutex` on JavaScript backends. `mutexAcquire` and `mutexRelease` would then just be no-ops on those backends, since they are single-threaded anyway.

Thank you for the feedback and the possible solutions.

I have decided to just let the client make the decision of when to utilize atomicModifyIORef, which seems like the simplest solution currently (this should be addressed via 0fb8ed7cf8c758bc039f37c0ab464d48fb40b6b3).

Might be cool to add some dummy primitives for the javascript backend in the future though.

Matthew-Mosior commented 2 months ago

@stefan-hoeck @gallais

Not sure what to do about the failing checks (pretty much the same issue for all of them):

Downloading single artifact
Error: Unable to download artifact(s): Artifact not found for name: ubuntu-installed-idris2-0.7.0-chez
        Please ensure that your artifact is not expired and the artifact was uploaded using a compatible version of toolkit/upload-artifact.
        For more information, visit the GitHub Artifacts FAQ: https://github.com/actions/toolkit/blob/main/packages/artifact/docs/faq.md
mattpolzin commented 2 months ago

RE failing checks, it may just be that GitHub broke home-directory resolution in their paths for artifacts. I am testing this theory out now.

[EDIT] I was wrong. But I did spot a suspicious looking "include hidden files: false" setting that might be causing this problem.

Matthew-Mosior commented 2 months ago

RE failing checks, it may just be that GitHub broke home-directory resolution in their paths for artifacts. I am testing this theory out now.

[EDIT] I was wrong. But I did spot a suspicious looking "include hidden files: false" setting that might be causing this problem.

Thank you for looking into this!

mattpolzin commented 2 months ago

Testing the hidden file theory now. Very promising given the recency of https://github.com/actions/upload-artifact/issues/602.

[EDIT] Looks good. Fix is up for PR.

mattpolzin commented 2 months ago

You're good to merge main into this branch when you get the chance and CI should work again.

Matthew-Mosior commented 2 months ago

You're good to merge main into this branch when you get the chance and CI should work again.

Sounds good, just merged main in!