-
Notifications
You must be signed in to change notification settings - Fork 0
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 Resolve
r for Playground
#196
Conversation
|
Regarding the Resolve API I imagine that we perhaps could do something like this: 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? |
4c4d1e0
to
1d04e25
Compare
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
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:
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:
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 |
7963bf1
to
05e44c7
Compare
79644c7
to
f762eee
Compare
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, 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 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 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. |
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. |
There was a problem hiding this 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
5c13e99
to
ed76c2e
Compare
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! |
There was a problem hiding this 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.
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: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 aReader
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
andjs-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:
pkgs:
-matching thing out to Core, so that Core gives aResolutionJob
containing the path to a module/name of the module, and where to look, so the implementations always are consistent.Cargo.toml
under Playground web bindings, there are a lot of features forweb-sys
).thread_local!
/RefCell
combo toMutex
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 fineweb_resolve
.This PR is drafted in this current state for you to check out if you want. If you compile the document, 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