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

Add Resolver for Playground #196

Merged
merged 20 commits into from
Apr 2, 2023
Merged

Add Resolver for Playground #196

merged 20 commits into from
Apr 2, 2023

Conversation

CMDJojo
Copy link
Member

@CMDJojo CMDJojo commented Mar 28, 2023

This PR adds a Resolve implementation for the playground. Resolves GH-195.

Currently, this is in early draft stage, and serves as a proof of concept. Package registry and local packages isn't available yet, to test it use something like: This PR is ready for review now, and you can test it out by compiling this in the playground:

[config]
import pkgs:hello

Lots of problems occurred when making this implementation. First of all, I was somehow unable to read the result into a array_buffer initially, forcing me to actually make a Reader implementation. I thought I could do that in JavaScript since it would be easier not having to worry about types, but since we are building with --target no-modules, see #152 for reasoning, we can't import JavaScript from Rust (otherwise that would have simplified things a lot). Then, the actual rust libraries used (web-sys and js-sys) had stuff hidden behind lots of flags and the implementations were hard to find. It was impossible to actually wait for the promises to resolve, I tried busy-waiting (we can't busy-wait since this is running in a single-threaded environment), I tried channeling (we can't since condvars aren't supported in wasm), I tried manually polling futures in Rust by using the definition (it required some libraries to get a waker, and still didn't work, it had the same behaviour as busy-waiting). The current solution I have now is to synchronously return empty byte arrays to each request, and making sure those packages aren't loaded, and when I actually get the packages, I load them later. This isn't a good solution but it does work.

Some of the things I want to fix before this PR is ready for review (this may take some time) is:

  • Change the Resolve trait to allow stuff to happen asynchronously. This will be beneficial for Cli as well, but I am still thinking of what the best way to do it. We could use callbacks with counters, but this would require us to have the compilation thing async and we have to make sure not to use condvars or busy-waiting, or get a weird stacktrace by multiple strange indirections. Or we could keep it as it is in the Playground, where the first compilation fails but subsequent succeed due to caching.
  • Figure out what type casts used are needed, which of them are safe (and can be unwrapped) and which may error.
  • Figure out what goes wrong when things go wrong, so we can handle errors better.
  • Maybe moving the pkgs:-matching thing out to Core, so that Core gives a ResolutionJob containing the path to a module/name of the module, and where to look, so the implementations always are consistent.
  • General clean-up, removing some of the old code that is not working at the moment, removing JS code, optimising features and imports (see Cargo.toml under Playground web bindings, there are a lot of features for web-sys).
  • Possibly change the thread_local!/RefCell combo to Mutex since it could actually, in the current state, crash if the main thread tries to access the context in the same time as a package is resolved (won't crash the other way around). Or, adding a fail-safe to the other part. Didn't really seem to work, since something makes it not thread-safe. We'll just have to hope that we don't mut borrow twice hehe, but I think it should be fine
  • Figuring out the best structural decomposition in web_resolve.
  • General clean-up (the code is actually very bad at the moment)

This PR is drafted in this current state for you to check out if you want. If you compile the document

[config]
import https://raw.githubusercontent.com/modmark-org/packages/main/hello.wasm

[hello]

, wait some seconds for the module to be downloaded, then re-compiling (by adding a space at the end or something), you will get "Hello world" from the Hello module downloaded from the internet.

See further comments for a round-up on the implementation

@github-actions
Copy link

github-actions bot commented Mar 28, 2023

PR Preview Action v1.3.0
Preview removed because the pull request was closed.
2023-04-02 16:39 UTC

@adelhult
Copy link
Collaborator

adelhult commented Mar 29, 2023

Regarding the Resolve API I imagine that we perhaps could do something like this:
(note that it is very much pseudo code and over-simplified but I think it gets the point across. I also think it's rather ugly having to manually ensure that the pending requests field is empty, perhaps we could encode this in the typesystem instead using the typestate pattern or something like that. eval<T, U, S: StatusComplete>(source: &str, ctx: Context<T, U, S>) )

trait Resolve {
    resolve(&str);
}

struct Context {
    // ...
    pending_requests: Vec<String>; 
}

impl Context {
   // ...
   add_missing(&self, path: &str, data: &[u8]) {/* ...  */}
}

impl Resolve for Playground {
    resolve(path: &path) {
        // ...

        fetch(path, |data| {
            CTX.add_missing(path, data);
            tell_playground_its_ready_to_be_recompiled();
        });
    }
}


eval(source: &str, ctx: &Context) -> Result<String, CoreError> {
    if !ctx.pending_requests.is_empty() {
        return Err(CoreError::WaitingForPackagesToDownload);
    }
    // ...
}

Also, if we don't want the trait implementation to just use a global context singleton we could wrap the context inside of a Arc<Mutex> and pass that together with the path perhaps?

@CMDJojo
Copy link
Member Author

CMDJojo commented Mar 30, 2023

Update on the state of this PR:

This was a lot harder than expected. With our Resolve API, we expect the results synchronously, which is all by itself not so good since they often require asynchronous work. The way we did it previously in the CLI was saying to tokio "By the way, we are gonna block now" and blocking the main thread until every request is resolved. This worked, but wasn't good at all. When (trying to) implement this for Web, it didn't work at all, since there aren't multiple threads. There is a spawn_local that spawns a background task, but I found no way of actually blocking the main thread until the other threads were complete. Some things I tried that didn't work was:

  • Getting an executor and awaiting it (didn't work, there are no such executors)
  • Manually polling it (we are missing a waker, and using a noop-waker and just polling in a loop doesn't work since we aren't doing it multi-threaded so no background tasks could proceed)
  • Sending a mutex lock and waiting for it to be released (didn't work for some reason, probably since main thread was blocked waiting)
  • Using mpsc (multiple-producer single-consumer) channels (which is possibly the "best" way to synchronize sync/async, and communicate between threads) (didn't work since condvars doesn't work on the web, which I first learned when actually doing the ENTIRE implementation and then I got an error in the last compilation step)

What I did to make it work was kinda ugly; I returned empty vectors and made sure to skip loading on empty vectors, then I did everything in the background and loaded everything into the context when I actually got the results. This was abusing the APIs so hard that I couldn't possibly feel good doing it.

This, in turn, prompted me to re-do the API. Which I did. Which was a lot of work. The main goal I had was to have the resolve not be like "Give me these packages NOW" but rather "Here are some packages I would love u to resolve, possibly multi-threaded". I decided to move out all the package-dependent things to a new PackageManger struct in Core for two reasons, the first being that we would need lots of fields to keep track of current jobs, failed jobs and so on, and the second being that we would possibly need Sync-references to it and it would be a bit overkill to force Context to be Send.

The new API looks like this:

pub trait Resolve {
    fn resolve_all(&self, paths: Vec<ResolveTask>);
}

impl ResolveTask {
    pub fn complete<E>(mut self, result: Result<Vec<u8>, E>)
    where
        E: Error + Send + 'static,
    {
        /* Implementation details */
    }
}

pub struct ResolveTask {
    // Private fields
    pub package: PackageID,
}

pub struct PackageID {
    pub name: String,
    pub target: PackageSource,
}

pub enum PackageSource {
    Local,
    Registry,
    Url,
}

An implementor of Resolve thus gets a list of ResolveTasks where each one contains a package to be resolved and a call-back function to report success or failure. The API is designed to be async-compatible, so implementors may decide to do it multi-threaded. So, this is a basic workflow of compiling a file:

  • Core gets a file to compile
  • It parses the file and asks its package manager if it got all the required packages
  • If not, it returns None (not an error, so we can see the difference) and asks Resolve to resolve the things
  • Then, the implementor of Resolve may decide what to do next (thus the control returns to the user of Context)
    • For CLI: The Resolver starts resolving the packages, and when done, sends a message back to the main thread with a mpsc 1-buffer channel, and then the CLI compiles the document again
    • For Web: The Resolver starts resolving packages, but does currently not recompile automatically
  • At following compiles, the PackageManger will additionally finalize the loaded packages, which includes compiling all packages received until now (and here loading errors may occur if, let's say, one of the resolved packages has a bad manifest)
  • If everything is correctly loaded, the document will be compiled and is retuned; if something is unavailible, None is returned yet again

Since each ResolveTask needs a mutable reference to the PackageManager, and since ResolveTask needs to be Send, Context holds an Arc to it. The implementation is working as intended now, but there are A LOT of junk left in the code due to all design attempts I did back and fourth, and some of the functions that should be in PackageManager isn't moved yet. However, the current state is actually working, and this document compiles both in the Playground and CLI, using the API above:

[config]
import https://raw.githubusercontent.com/modmark-org/packages/main/hello.wasm

[hello]

Note: Since automatic re-compilation isn't implemented for the web yet, you need to re-compile manually by adding a space or something to the document

There are a lot left to do in terms of error handling, optimising usages of Arc to avoid too many clones etc and general clean-up. This PR became much larger than anticipated (see added/removed lines), but I hope that is okay. One thing to note that there is no more "borrowing references to PackageInfo" and such since we need PackageManager to have a Sync/Send API and borrows and references can't really be checked in such environments (without pinning everything), so we'll have to do with Arc-s.

For you @adelhult who had ideas for the API, please check if my API definition is okay. I thought it would be better to actually store a reference to PackageManager in ResolveTask to reduce congestion and to simplify the API a bit. Also, I decided to not have a CoreError::NotReadyYetBecauseOfMissingPackages error since I think it is important to make a distinction between "Error, something went wrong, there is probably an error in your document" and "I have sent an asynchronous task, and I have no document for you yet". I hope that makes sense

@CMDJojo CMDJojo force-pushed the cmdjojo/GH-195/0 branch 3 times, most recently from 7963bf1 to 05e44c7 Compare March 30, 2023 21:27
@CMDJojo CMDJojo marked this pull request as ready for review March 31, 2023 13:18
@CMDJojo
Copy link
Member Author

CMDJojo commented Mar 31, 2023

This PR is ready for review now. It is very large (much larger than the issue would hint at), so if you think we for the sake of things should make some more issues and link them we can do that (but I think its fine anyways). The async nature of everything requires consumers to check if the document is ready to recompile, and if not, wait until it is. This is done in two different ways for CLI and Playground:

For the CLI: Resolving packages occurs in a separate thread, and that thread has a low-buffer channel back to the main thread. If the main thread detects that packages needs to be resolved first, it listens to the channel and on a message, tries to recompile. If it still fails, some packages probably failed to resolve, and it attempts the procedure once again. It can do a maximum of 3 retries before abandoning.

For the Playground: Resolving packages occur as background tasks in the worker (but the single-threaded nature of Javascript means that this probably is done in the same thread). We have a global counter for how many packages there are left to resolve, and when it is 0, recompile() is ran. This should send a message to the main task but I found no way of actually doing it. We can't bind JS and use that from Rust, and somehow it doesn't seem to be a postMessage in the globals for the web_sys library (there probably is but I found none). However, the way I implemented it is with a polling function is_ready_for_recompile→bool that is polled every 200ms for at most 5 seconds. If it reports true, the document will be recompiled. Note that we actually compile the document one time first that may fail, so if we add [config] import xyz the playground would display Error: No Result for a short while until that package is resolved and the whole thing is recompiled.

As an additional note, there were problems configuring Cors properly for the playground web request. Everything seemed to work when I didn't configure anything, but setting the mode to Cors and allowing all origins would still give access errors on redirects and such. Therefor, it would be great if our registry uses direct links to the wasm files (raw.githubusercontent.com) rather than links that would redirect (github.com). If anyone wants to take a shot at resolving it, it would be great, however I have read much Cors documentation and tried multiple different configurations and headers, and I was unable to solve it.

As another additional note, even though Mutex and atomics is available when compiling to Wasm, it doesn't seem to work to resolve packages synchronously. That is the reason for the spawn_local(async{fetch_local(task)}) in web_resolve. I know for a fact that it doesn't get into any deadlocks, but I get an error that recursive mutex locks aren't supported and I am not so sure about why that error would occur. We have a DenyAllResolver which just drops all tasks in sync and that works file for the CLI and doesn't get into any deadlocks. I would blame this on some problem with the generated Wasm code, but as I was unable to resolve it, I am spawning a local task to reject the job in async, and that works fine.

I think the PR is ready for review now. It is quite large, so even if you don't find the time to actually go through and check all code, just asking questions and commenting on things you think look strange/want me to explain/that you think is redundant would be much appreciated.

@axoxelol1
Copy link
Collaborator

axoxelol1 commented Mar 31, 2023

Nice work! I tested importing the hello world example in CLI and playground on my machine, both worked well. I skimmed through the code but like you said it is a very large PR I think my time might be better spent on the report. Nice job overcoming a lot of the difficulties encountered.

I won't approve it just yet want to give @adelhult a chance to review has he might have more opinions.

Copy link
Collaborator

@adelhult adelhult left a comment

Choose a reason for hiding this comment

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

Amazing work! I really like the ResolveTask-API and it is great to have a less ad-hoc solution for an async package resolver. I left a lot of comments but they are mostly related to outdated naming of PackageStore and some wishes for clearer names in some other cases here and there

cli/src/main.rs Outdated Show resolved Hide resolved
cli/src/main.rs Outdated Show resolved Hide resolved
cli/src/main.rs Outdated Show resolved Hide resolved
cli/src/package.rs Outdated Show resolved Hide resolved
core/src/context.rs Outdated Show resolved Hide resolved
core/src/package_store.rs Outdated Show resolved Hide resolved
core/src/package_store.rs Outdated Show resolved Hide resolved
core/src/package_store.rs Outdated Show resolved Hide resolved
playground/static/compiler.js Outdated Show resolved Hide resolved
playground/static/compiler.js Outdated Show resolved Hide resolved
@CMDJojo CMDJojo force-pushed the cmdjojo/GH-195/0 branch from 5c13e99 to ed76c2e Compare April 2, 2023 14:31
@CMDJojo
Copy link
Member Author

CMDJojo commented Apr 2, 2023

Thank you for your comments @adelhult . I have now reviewed all comments and changed the code accordingly; please check the most recent commit for the changes and see if you are happy with them. Feel free to comment on more things that you found after your review, or changes that you think I did poorly!

Copy link
Collaborator

@adelhult adelhult left a comment

Choose a reason for hiding this comment

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

Great work! Thanks for the changes.

@CMDJojo CMDJojo merged commit d3e6232 into main Apr 2, 2023
@CMDJojo CMDJojo deleted the cmdjojo/GH-195/0 branch April 2, 2023 16:39
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.

Add Resolver for Playground
3 participants