search

zamazan4ik / aws-gamelift-server-sdk-rs

MIT License
5 stars 1 forks source link

Have ProcessParameters callbacks return Futures so they can be awaited #3

Closed Luminoth closed 2 years ago

Luminoth commented 2 years ago

This is the other change I mentioned in https://github.com/zamazan4ik/aws-gamelift-server-sdk-rs/pull/2. It makes the ProcessParameters callbacks return futures so they can be awaited at the caller rather than having to spawn tasks internally. It's debatable I think if it's actually cleaner since it requires the pinned Box but I think if async callbacks ever get stabilized that would clean up nicely.

The other big thing obviously is that this is a breaking change. I don't know how much impact that would actually have on anyone and I think it's pretty easy to adjust for. Either way, here it is if it seems worth taking.

zamazan4ik commented 2 years ago

Thanks for the PR. Just want to clarify one thing: why do we need pin here? Is there any way for avoiding pinning here?

The other big thing obviously is that this is a breaking change. I don't know how much impact that would actually have on anyone and I think it's pretty easy to adjust for. Either way, here it is if it seems worth taking.

I don't think that it's a big deal since we don't have many library users for now. And anyway version number for the library will be increased.

Luminoth commented 2 years ago

Futures have to be pinned in order to be polled - https://rust-lang.github.io/async-book/04_pinning/01_chapter.html - and I believe since async closures aren't stable yet - https://github.com/rust-lang/rust/issues/62290 - it forces that awkward Box::new(|| Box::pin()) since you have to box the closure and then pin the boxed future it returns. I'm not the greatest at this tho so there might be a way to better hide some of this, like I believe https://docs.rs/warp/latest/warp/filters/struct.BoxedFilter.html is how warp hides the underlying magic.

zamazan4ik commented 2 years ago

Thanks for the explanation - I definitely need to read the Async book more precisely next time. Seems good to me.

Last question - do you have any additions in the queue? I can just wait for them before the next release.

Luminoth commented 2 years ago

Nah, no more changes for now, that was the last one I've got. I appreciate you taking those in, it actually really makes my life easier having them.

zamazan4ik commented 2 years ago

So, I will create a new release. If I will forget to do it (because I am a usual human) - just ping me here :) Thanks for your contributions!