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

Rewrite async modules to non-experimental coroutines #154

Closed
czyzby opened this issue Sep 3, 2018 · 10 comments
Closed

Rewrite async modules to non-experimental coroutines #154

czyzby opened this issue Sep 3, 2018 · 10 comments
Assignees
Labels
async Issues of the ktx-async module dev Miscellaneous issues and dev tasks update Issues related to updates of project's dependencies
Milestone

Comments

@czyzby
Copy link
Member

czyzby commented Sep 3, 2018

Experimental coroutine APIs are deprecated and yield errors in Kotlin 1.3.

@czyzby czyzby added dev Miscellaneous issues and dev tasks update Issues related to updates of project's dependencies async Issues of the ktx-async module labels Sep 3, 2018
@czyzby czyzby added this to the backlog milestone Sep 3, 2018
@czyzby czyzby self-assigned this Sep 3, 2018
@czyzby
Copy link
Member Author

czyzby commented Sep 20, 2018

ktx-async uses some APIs deprecated as of 0.26.1. The full test suite passes for now, though.

@lokeshj
Copy link

lokeshj commented Oct 30, 2018

@czyzby Now that coroutines are 1.0, could you please take a look at this again?

@czyzby czyzby modified the milestones: backlog, 1.9.8 Oct 30, 2018
@czyzby
Copy link
Member Author

czyzby commented Oct 30, 2018

@lokeshj Sure, but probably no sooner than this weekend.

@czyzby czyzby modified the milestones: 1.9.8, 1.9.9 Nov 17, 2018
@jcornaz
Copy link
Contributor

jcornaz commented Dec 3, 2018

ktxAsync should be an extension on CoroutineScope as well to be consistant with other coroutine builders of kotlinx.coroutines and to make life-cycle mangements easier.

@czyzby
Copy link
Member Author

czyzby commented Dec 4, 2018

The develop branch is compatible with 1.0.1 coroutines API as of merging #170, although I still think ktx-async should be rewritten to leverage new coroutine APIs. See #169.

@jcornaz
Copy link
Contributor

jcornaz commented Dec 8, 2018

I know it is likely to be solved when you'll rewrite the async module, but I'd like to report here a bug I've fount using 3.9.9-SNAPSHOT today: AssetStorage.load is not thread safe.

Example:

ktxAsync {
  val texture1 = async { assetStoreage.load<Texture>("texture1.png") }
  val texture2 = async { assetStoreage.load<Texture>("texture2.png") }

  addScreen(MyScreen(texture1.await(), texture2.await())
  setScreen<MyScreen>()
}

With the code above, I experienced visual glitch like inverted textures, or getting only partial or scaled texture. Removing usages of async solved the problem.

Thread safety is an implicit contract of any suspend function. If necessary the function may use Mutex.withLock, withContext or channels to achieve its thread safety. But I always expect from a suspending function to be callable from any context.

@czyzby
Copy link
Member Author

czyzby commented Dec 8, 2018

I know, the new solution uses a single assets mutable collection protected by a Mutex.

@czyzby
Copy link
Member Author

czyzby commented Jan 24, 2019

Just wanted to let you know that I'm currently dealing with work and the end of semester at my uni, so I had to postpone this "a little". I should be done in a few weeks.

@czyzby czyzby mentioned this issue Jan 30, 2019
10 tasks
czyzby added a commit that referenced this issue Feb 5, 2019
@czyzby
Copy link
Member Author

czyzby commented Feb 5, 2019

@jcornaz I finished the documentation of the new ktx-async module and pushed the overhaul to the develop branch. I decided to remove the AssetStorage, since I don't think it should be strictly connected with the core coroutines utilities and I have to admit I'm not done with that one yet. Could you take a look at the new API?

@czyzby
Copy link
Member Author

czyzby commented Feb 5, 2019

I'm closing this issue for now, AssetStorage will be implemented separately in 1.9.9-b2.

#182

@czyzby czyzby closed this as completed Feb 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async Issues of the ktx-async module dev Miscellaneous issues and dev tasks update Issues related to updates of project's dependencies
Projects
None yet
Development

No branches or pull requests

3 participants