Adding in Promises

[Pauan]

Fixes #72

@koute @CryZe This is untested and is definitely not ready to merge.

But I wanted to at least get started on adding in Promise support to stdweb. This pull request adds in the ability to convert any JS Promise into a Rustified Futurified version of that Promise. This conversion happens automatically with the TryFrom<Value> trait.

Please look this over and let me know if you see anything weird, or anything that can be improved.

[Diggsey]

There are a few problems here:

• You're sending values of type Result<T, JsError> over the channel, but then you're expecting to receive a T at the other end.
• There's no way to poll the resulting future because there's no reactor implementation that's compatible with stdweb.
• The inner future does not need to be boxed.
• You can use futures::unsync::onsehot.

Before jumping into implementing a promises -> futures adaptor, I think it would be useful to implement a futures-compatible reactor on top of the javascript event loop, that way it's possible to test that everything is working.

[Pauan]

@Diggsey You're sending values of type Result<T, JsError> over the channel, but then you're expecting to receive a T at the other end.

I have no idea what you're talking about. It uses .and_then( future::result ) to convert the Result<T, JSError> into T. I may not have tested the code, but it does type-check.

There's no way to poll the resulting future because there's no reactor implementation that's compatible with stdweb.

Yup! That'll need to be added.

The inner future does not need to be boxed.

Yes it is needed. You can't use impl Future in structs, and if you try to spell out the full type it doesn't work because FnMut traits do not have a fixed size.

You can use futures::unsync::onsehot.

That's a good idea, I'll do that.

Before jumping into implementing a promises -> futures adaptor, I think it would be useful to implement a futures-compatible reactor on top of the javascript event loop, that way it's possible to test that everything is working.

Indeed, which is why I said this is untested and not ready to merge. It is a work in progress.

[Diggsey]

I have no idea what you're talking about. It uses .and_then( future::result ) to convert the Result<T, JSError> into T. I may not have tested the code, but it does type-check.

Ah yes

Yes it is needed. You can't use impl Future in structs, and if you try to spell out the full type it doesn't work because FnMut traits do not have a fixed size.

You can just store the oneshot::Receiver future as your "inner" future, and do your mapping in the implementation of poll - if you're implementing Future manually, you don't have to stick to using the combinators all the time.

[Pauan]

@Diggsey You can just store the oneshot::Receiver future as your "inner" future, and do your mapping in the implementation of poll - if you're implementing Future manually, you don't have to stick to using the combinators all the time.

You're right that is possible, but I was concerned about potential performance issues, since it would be rebuilding the Future every time poll is called. If the performance is identical, then I agree that moving the combinators into poll is a good idea.

As for not using combinators at all, yes that is possible, but it would require Rc + RefCell shenanigans, which is basically what channel is doing internally anyways, so it's nice to be able to re-use existing well-tested battle-hardened APIs rather than rolling my own.

A good example of not using the combinators would be implementing Future for setTimeout, which is what stdweb-io does. It doesn't need to use the combinators at all. But that only works because setTimeout never throws errors. Which means it won't work for converting Promises into Futures, because Promises can throw errors.

[Diggsey]

it would be rebuilding the Future every time poll is called.

What? No, take a look at how the future combinators are actually implemented, you would just do this:

fn poll( &mut self ) -> Poll< Self::Item, Self::Error > {
    self.future.poll().map_err( |x| JSError::new( x.description() ) ).and_then( future::result )
}

There's no performance penalty here: this is exactly how Future::and_then and Future::map_err are implemented.

As for not using combinators at all, yes that is possible,

I think you are misunderstanding the terminology - combinators are the methods used to compose futures together, eg. and_then, map_err, etc. They typically take some number of futures/closures as input and produce a new future. They serve as convenience methods to avoid having to manually implement Future. In my example above, I am not using the future combinators because I have implemented poll() manually using the similar methods on "Result".

Not using oneshot at all would be an option, but I agree with you that it's not worth it, because you'd be re-implementing much of its functionality.

[Pauan]

@Diggsey What? No, take a look at how the future combinators are actually implemented, you would just do this:

Yes I know what you were referring to.

There's no performance penalty here

I will verify that the performance of the compiled binary code will be just as fast (or faster). I don't judge performance based upon source code.

I think you are misunderstanding the terminology

I am not misunderstanding anything, I know exactly what "combinator" means in this context.

I am not using the future combinators because I have implemented poll() manually using the similar methods on "Result".

You are using the combinators within the implementation of poll. That is what I meant by "not using the combinators at all".

[CryZe]

Alright, so I'm not quite sure why we are boxing the oneshot channel's receiver, as there should be absolutely no reason for doing so. The futures I have in stdweb-io don't do any of this either. Additionally you lose all the OIBITs if you use trait objects, so I wouldn't use them unless there's a good reason for them, which I don't quite see yet.

Regardless, most of this PR looks pretty good to me. This is still missing an Executor. You can take a look at the one I made, or I can make a PR after this that brings the Executor.

[Pauan]

@CryZe Additionally you lose all the OIBITs if you use trait objects, so I wouldn't use them unless there's a good reason for them, which I don't quite see yet.

Yes, I definitely don't like using Box, I only did it because I had to in order to make it compile.

This is still missing an Executor. You can take a look at the one I made, or I can make a PR after this that brings the Executor.

Sure, I'll take a deeper look at your code, I'll let you know if I can't figure it out.

---

@Diggsey I understand now what you meant about "using the combinators on Result".

However, I tried moving the combinators into poll, but:

error[E0631]: type mismatch in function arguments
   |
   |         self.future.poll().map_err( |x| JSError::new( x.description() ) ).and_then( future::result )
   |                                                                           ^^^^^^^^
   |                                                                           |
   |                                                                           expected signature of `fn(futures::Async<std::result::Result<A, JSError>>) -> _`
   |                                                                           found signature of `fn(std::result::Result<_, _>) -> _`

error[E0507]: cannot move out of borrowed content
   |
   |         self.future.map_err( |x| JSError::new( x.description() ) ).and_then( future::result ).poll()
   |         ^^^^ cannot move out of borrowed content

It seems there aren't any combinators that will work for this situation, so I decided to just use manual pattern matching:

fn poll( &mut self ) -> Poll< Self::Item, Self::Error > {
    match self.future.poll() {
        Ok( Async::Ready( Ok( a ) ) ) => Ok( Async::Ready( a ) ),
        Ok( Async::Ready( Err( e ) ) ) => Err( e ),
        Ok( Async::NotReady ) => Ok( Async::NotReady ),
        Err( e ) => Err( JSError::new( e.description() ) ),
    }
}

This should be much better and faster than using Box + combinators.

[Pauan]

I added in an Executor and also documentation. I think this is pretty close to being mergeable.

I did some light testing in the examples/promise folder, and everything seems to work correctly!

Thanks a lot @CryZe for your Executor implementation! I probably wouldn't have been able to create one by myself. I mostly copied your code as-is, but I did a bit of refactoring on it. You can see the code in webcore/promise_executor.rs

[Pauan]

I added in a new spawn_print method, which is similar to spawn except that it prints the error to the console (if an error occurs).

This is a necessary convenience, because the alternative is to tell the user to handle the errors themself, and it's easy to get error-handling wrong (e.g. accidentally silently discarding errors, printing the errors in a bad format, etc.)

However, I don't like the type of spawn_print. It requires the error type to be stdweb::web::error::Error, which is fine for PromiseFuture (because the error type of PromiseFuture is always stdweb::web::error::Error), but that won't be true for other Futures.

The reason it requires stdweb::web::error::Error is because the browser can pretty print JavaScript errors better than Rust can (browsers display the stack trace, browsers understand source maps, etc.)

I've thought about possibly adding in a new trait which can be used to give special behavior to stdweb::web::error::Error while still allowing for other error types to work as well.

Any ideas or suggestions for how to handle this situation are appreciated.

[Pauan]

My current preferred solution is to add in this impl:

impl From< stdweb::web::error::Error > for () {
    #[inline]
    fn from( error: stdweb::web::error::Error ) -> Self {
        js! { @(no_return)
            console.error( @{error} );
        }

        ()
    }
}

And now it's possible to use .from_err() to convert from stdweb::web::error::Error to () (with correct error printing):

PromiseFuture::spawn(
    create_some_future()
        .from_err()
);

I don't like that it has side effects in the implementation of from, but the alternative would be to create a new trait.

[Pauan]

After thinking about it, I decided on this solution:

I added in a print method to stdweb::web::error::Error (this can be generalized to a trait later, if needed):

impl Error {
    #[inline]
    pub fn print( &self ) {
        js! { @(no_return)
            console.error( @{self} );
        }
    }
}

I also removed PromiseFuture::spawn_print. Now the user can do this:

PromiseFuture::spawn(
    create_some_future()
        .map_err(|e| e.print())
);

I think this is ready to merge now. @koute @CryZe @Diggsey requesting a final review on this.

[CryZe]

The huge todo comment in my Executor still needs to be fixed. Basically a Future could resubmit itself while it is currently executing, at which point the RefCell will deadlock (panic), which is probably not what we want. The solution would be to try_lock the RefCell and on unsuccessful locking, queuing up the future for automatic re-execution when the current execution is done. That should most likely either be a Cell<usize> or Cell<bool>. I'm not quite sure if we need to count how many times it tried to resubmit, but I think we should probably just count.

[Pauan]

@CryZe How would a Future resubmit itself? Ownership is passed into spawn, right?

[CryZe]

The Future gets the Task Notifier Handle during its execution, at which point at any time it can call that and will deadlock, if it's still executing. I've at least encountered one situation where this did indeed happen. Usually it's not a problem with other executors, as they just queue up the futures to be reexecuted, but here we just lock up, as with this executor, we try to immediately execute everything.

[Pauan]

@CryZe I see, I'll try and fix it. Though I don't fully understand the internals of the Future/Executor system, so it would be very helpful if you have a small test case to reproduce it!

[CryZe]

If you can give me edit rights on this PR I can fix it real quick if you want. (Alternatively I can do a PR on the PR branch)

[Pauan]

@CryZe I made you a collaborator on Pauan/stdweb. The code is in the promises branch.

[CryZe]

Alright, I think I did it. You may want to run some more testing on this, as there was a lot more subtleties involved than I thought.

[Pauan]

@CryZe Why is it using Cell, rather than simply mutating it directly with &mut?

Also, what are you using to test that it's correct?

[CryZe]

This needs to be behind a Cell / Lock / Atomic type in order to allow sharing + mutating it. Everything else is undefined behavior. (The pointer types actually may turn off those compiler optimizations already, but I'd rather not introduce some hidden undefined behavior somewhere. And this indicates that synchronization is necessary if WebAssembly ever gets proper threading. So Cell<usize> -> AtomicUsize).

I didn't do much testing yet. I'll probably test it a bit more tomorrow.

[Pauan]

@CryZe Ahhh, I see, having multiple *mut pointers to the same memory location is undefined behavior. And the reason you're using Cell rather than RefCell is because Cell is faster. And the reason Cell can get away without runtime borrow checking is because it only allows Copy types (and it doesn't allow references to the inner data).

The changes look good! I'll also see if I can get some tests going tomorrow.

[Diggsey]

Why not just keep the name resolve?

[Pauan]

Because that name is super confusing.

If I see the name promisify I have a pretty good idea of what it does (it probably converts something into a Promise).

If I see the name resolve, I would never in a million years think that it converts something into a Promise. I would probably assume that it is somehow mutating an existing Promise or something (which isn't what it does).

Also, I have thought about changing it so that the promisify function only works on fake Promises, not arbitrary values, so its implementation won't be as simple as just Promise.resolve(value)

I'm open for suggestions on changing its name, but resolve is just a bad name for what it does. It's a bad name in JavaScript and an even worse name in Rust.

[Diggsey]

There is value in sticking to the original method names though, and I don't think promisify is any more useful a name - you can figure that much out by the type signature.

As a compromise, how about Promise::resolved(..) - this at least better indicates that it's constructing a new, resolved promise rather than modifying an existing one, while being very close to the original name.

[Pauan]

I can compromise with Promise::resolved, though I do worry about the confusion for people who are used to JavaScript (they need to remember that it ends in d in Rust, but doesn't in JavaScript)

I think for that reason Promise::resolve would be better than Promise::resolved, though both are quite bad.

The more I think about it, the more I prefer the solution of changing Promise::promisify to only convert fake Promises, and then the name makes more sense. Or perhaps rename it to Promise::convert, I'm still open to name suggestions.

[Pauan]

@CryZe I don't think the executor is correct. When I tried testing the code, I was able to get it to dead-lock, because the executor loop never yields to the JavaScript event loop.

I think we need to insert a setTimeout somewhere or other to let the JS event loop breathe a little.

[Pauan]

I think I fixed the deadlock, but I'm not sure if it's memory safe.

I added in a MyFuture struct to examples/promise/main.rs which demonstrates the deadlock with the old code (it works fine with my changes).

[Pauan]

It seems it isn't memory-safe: I get an "already borrowed: BorrowMutError" error, which I don't think ever happened with the old code.

[Pauan]

I think the reason it was causing problems was because drop_id was being run before the setTimeout finished, and drop_id then dropped the SpawnedTask, so the setTimeout callback ended up reading into dropped memory.

I think I fixed it by adding in an Rc<Cell<bool>> which indicates whether the SpawnedTask is alive or dropped, and then the setTimeout callback checks that before calling run

[Pauan]

Well, if there aren't any other objections, then @koute could you please review and merge this?

The remaining issues (adding in web APIs like setTimeout, making proper unit tests, and moving Error) will be fixed in a different pull request.

[koute]

Thanks a lot for all of the great work guys!

[koute]

Could we remove this method?

I understand the need to have convenience methods, but I'd prefer if we'd just wrap console.error itself so that you could do console::error(e).

[Pauan]

That's fair, though I had kind of figured that console.error(e) was a placeholder, and we might implement better error handling in the future, in which case having e.print() would be nice. But I don't have a particularly strong opinion about it, so I don't mind removing the print method.

[Pauan]

Since we don't currently have a console::error function, I'd rather leave this in for now. Afterwards I'll make another pull request which adds in console::error and removes print

[koute]

I think accepting a &Reference here would be better than a Value. (This will always return None for anything which isn't a Reference anyway, so I think it's better to statically signal that in the type signature instead of pretending that it can accept any Value.)

[koute]

Also, perhaps something like from_nonstandard_promise would be a better as a name? (Since it's more descriptive that way and as you've wrote in the comment yourself - it won't be used very often anyway.)

[Pauan]

But if it accepts Reference then you would need to do something like this:

Promise::convert(js!( return ... ).try_into().unwrap())

...which is annoying, verbose, and slower. And it doesn't really help, because even with a Reference it still might return None, so you don't gain any real static type safety.

Also, I've noticed that sometimes objects get turned into Object (not Reference), and the Promise::convert function works with any object which happens to have a then method (even Arrays!). So I think using Value is correct.

[koute]

Also, I've noticed that sometimes objects get turned into Object (not Reference)

This is actually a very good point, thank you! Hmm...

The original idea was that Object objects represent a plain, dumb, JavaScript object for holding data, but unfortunately a lot of "fat" objects (e.g. like Bluebird promises) in the wild seem to be indistinguishable from those which are just simple {key: value} containers for data. I'm thinking that perhaps we should stop treating Objects and Arrays in a special way, make them a normal #[derive(ReferenceType)] types and only have a Reference variant in Value.

I'm starting to really like this idea; then the problem of promises (and other kinds of objects) being converted to an Object will disappear.

And it doesn't really help, because even with a Reference it still might return None, so you don't gain any real static type safety.

(Assuming we'll remove Value::Object variant.)

1. Yes, but, conversely if someone already has a Reference they have to convert it to a Value now to pass into this function.
2. The types are there not only for guaranteeing type safety, but also to serve as documentation.
3. Casting a Value to a Reference is relatively cheap as it's just a match.
4. You could also convert it this way: js!( ... ).as_reference().and_then( Promise::convert ).unwrap(), which I don't think is that bad?

[Pauan]

If we remove the Object and Array variants, then yes I agree it would make more sense to make it Reference. But it's still a lot more verbose than using Value, so I'm not sure.

I'll leave it as Value for now, with a TODO note to change it later (if we decide to remove Object and Array variants).

[koute]

This should be cfg-gated with the futures feature; ditto for other futures-related code.

[Pauan]

What about the pub use webcore::promise::{Promise, PromiseFuture}; code in lib.rs? Should that also be cfg-gated?

Also, is it enough to only cfg-gate PromiseFuture, and not the to_future method?

[koute]

Both PromiseFuture and to_future need to be feature gated. Promise itself should not. (You can split the pub use line in lib.rs into two lines.)

[koute]

The error handling needs some work here.

1. I don't think using Error (as in - the JavaScript type Error) here is appropriate, as technically you can throw things which are not Error objects in JavaScript. It should be possible for the user to be able to retrieve the original value from promise's rejection.
2. It would be nice to figure out how to fit the ConversionError in this somehow. Perhaps convert it to a JavaScript TypeError? (This issue is also relevant in many other places across stdweb when we currently panic, but where I'd like to eventually throw a JavaScript exception when a type conversion fails.)

[Pauan]

Ah, good catch, you're right that technically any type can be an error, so it should be Result<Value, Value> (which can be auto-coerced into something more precise by the user if they desire).

It would be nice to figure out how to fit the ConversionError in this somehow.

What do you mean? It doesn't panic, it converts the ConversionError into a JavaScript Error. So what are you suggesting? To convert it into a JavaScript TypeError instead?

[koute]

What do you mean? It doesn't panic, it converts the ConversionError into a JavaScript Error. So what are you suggesting? To convert it into a JavaScript TypeError instead?

Yes, that's what I'm suggesting, since essentially a ConversionError is our counterpart of JS' TypeError. Another way to handle this would be to have an enum with two variants: one would be a Value, and another would be... something else (perhaps even the ConversionError if we make its innards private) which would signify a type error but without converting it to a JS type (keeping it purely in the Rust land).

I'm not sure which one would be better, but I'm probably leaning towards the first one's direction.

[Pauan]

I tried changing it to allow for arbitrary types for the error, but I got a lot of type errors (that I wasn't able to fix), so for now I'm just going to .unwrap() the ConversionError errors, but I'll add a TODO note to fix it later.

[Pauan]

@koute I changed PromiseFuture to have two type arguments: one for the success type and one for the error type. In other words, it's now possible to use arbitrary error types with PromiseFuture.

Which also means it's now possible to be more precise with the error type (e.g. PromiseFuture<String, DomException>)

I also renamed Promise::convert to Promise::from_thenable, since "thenable" is the standard JavaScript term for "something which has a then method".

I also fixed the cfg-gating (and verified that it works). It was easier to just move the futures-specific stuff into a promise_future.rs file, rather than trying to cfg individual statements.

Things to do after this pull request is merged:

• Add in console::error and remove Error::print

• Move Error into webcore

• Remove Object and Array variants from Value, and change Promise::from_thenable to accept a &Reference instead of Value

• Add in Futurified versions of some web APIs (e.g. setTimeout, etc.)

• Make proper async unit tests, rather than using examples/promise

• Low priority, but it would be nice to figure out a way to avoid .unwrap()

I'll do as many of these as I can, but anybody else can do them if they want to.

[koute]

Ok, I'll merge it in as-is, but as discussed this still needs some work so if by the time I'll be releasing 0.4 (which will hopefully be soon-ish, I still have a few APIs go to through) this won't be ready I'll have to temporarily make it private.

Thanks for the great work!

[koute]

@Pauan FYI, I've just got rid of the Object and Array variants in Value!