Skip to content
This repository has been archived by the owner on Apr 14, 2023. It is now read-only.

Note on resources, and undefined behavior inside the cycles (long term planning) #5

Closed
cg-jl opened this issue Aug 8, 2021 · 8 comments · Fixed by #24
Closed

Note on resources, and undefined behavior inside the cycles (long term planning) #5

cg-jl opened this issue Aug 8, 2021 · 8 comments · Fixed by #24
Labels
bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request

Comments

@cg-jl
Copy link
Contributor

cg-jl commented Aug 8, 2021

Look at the below snippet:

@name == 1 {
   delete(@path)
   shell(my-useful-check @path)
}

This snippet triggers undefined behavior, as it's using a resource potentially destructively (delete will certainly do, my-useful-check might do). You don't know which one will error out, if delete or shell. It could be that shell finds the file while delete is still working on it, and you never know what will happen next.

This issue is meant to open a debate. How shall we mitigate this issue? I've thought of three options, let me know if you have any more:

  • let the undefined behavior happen (as it would in the current state) but document it (i.e say which functions are potentially destructive). This might be a good thing to do while we're developing a more useful idea into the project, to advise against known issues.
  • staticly analyze the code, and trigger an error before it's run if there are calls within the same cycle to functions which will potentially destroy the resource (using the resource would mean passing the path, or the name, to a create/delete/shell function). This seems to me as the best one of the three.
  • silently modify the code before it's run, moving each call to their own separate cycle, and probably issuing a warning that those functions won't execute concurrently because they're trying to own the same resource at one. This is not a great thing to do as you're probably hindering performance through separating the functions and actually don't know which of the functions would be desired to run first.

Final note: I don't think we should start working on this immediately, but instead come up with different ideas or expand one of them as the project matures.

@Alonely0
Copy link
Owner

Alonely0 commented Aug 8, 2021

Interesting. Yeah, I thought about that, in fact, that was the whole point of the cycles, or at least, what initially made me create them, but after creating them I saw its potential utility was bigger than just avoiding undefined behavior.

I agree with you in the best solution, not the most easy to implement, but still the best one.

As long as this is still something we are concerned about, we should have it document it, as you said.

I'll create a separate branch later, so we can start working on this. Feel free to fork it and start working on the solution

@Alonely0 Alonely0 added bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request labels Aug 8, 2021
@Alonely0
Copy link
Owner

Alonely0 commented Aug 8, 2021

Bug because (can) cause undefined behavior, documentation because is not warned in the docs, enhancement because the solution will prevent undefined behavior and warn the user.

Alonely0 added a commit that referenced this issue Aug 8, 2021
@UE2020
Copy link

UE2020 commented Aug 16, 2021

let the undefined behavior happen (as it would in the current state) but document it

If it's documented, it's not really undefined anyways.

@Alonely0
Copy link
Owner

Alonely0 commented Aug 16, 2021

in fact no. it's undefined behavior because you cant predict what will happen, you just know that an error will happen

@cg-jl
Copy link
Contributor Author

cg-jl commented Aug 17, 2021

If it's documented, it's not really undefined anyways.

It is undefined behavior to attempt to use and modify a resource by more than one process/thread at the same time, it's also called a data race. Doesn't matter wether you know it's there or not, it's still going to happen and produce undefined results when executed. Rust prevents data races with memory but not with files in the disk that can only be affected with syscalls.

@Alonely0 Alonely0 pinned this issue Aug 17, 2021
@cg-jl
Copy link
Contributor Author

cg-jl commented Sep 7, 2021

I feel like refusing to run would be excessive, since then the user has no way to do two moves of different files simultaneously that don't conflict between themselves, for example. I propose a new idea: warn and give a prompt (to continue) so the user can revise it. We could have a flag --bypass-checks=ub to bypass the undefined behavior checks and prompt, e.g in case the user has already proved that it is impossible to have UB given the context in which Voila runs. If such a warning system were going to be implemented, a --warn=error could be a nice flag as well, but that'd be another topic.

@Alonely0
Copy link
Owner

Alonely0 commented Sep 7, 2021

yes I was considering the idea of the flags

@Alonely0
Copy link
Owner

With the new parser this should be easier to implement. I'll start working on it

@Alonely0 Alonely0 linked a pull request Sep 20, 2021 that will close this issue
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants