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

Mark GlobalScope as delicate API and rewrite coroutine basics doc without it #2637

Merged
merged 17 commits into from Apr 20, 2021

Conversation

elizarov
Copy link
Contributor

@elizarov elizarov commented Apr 8, 2021

Also, remove the tutorial variant of the basic coroutine introduction as it essentially duplicates the content of the
basic coroutine introduction, albeit with "how to create a project" additional information, which does not seem to be appropriate in this section at all.

Fixes #2122

…hout it

Also, remove the tutorial variant of the basic coroutine introduction as it essentially duplicates the content of the
basic coroutine introduction, albeit with "how to create a project" additional information, which does not seem to be appropriate in this section at all.

Fixes #2122
@qwwdfsad
Copy link
Member

qwwdfsad commented Apr 8, 2021

Please update api dump

@mhernand40
Copy link

mhernand40 commented Apr 8, 2021

Excited to see this! Thank you.

Curious what the fate is for some of the reactive stream interop APIs such as rxSingle, rxMaybe, rxCompletable, etc. which internally leverage GlobalScope in order to break out of structured concurrency? Will these APIs be marked as delicate as well?

@elizarov
Copy link
Contributor Author

@mhernand40

Excited to see this! Thank you.

Curious what the fate is for some of the reactive stream interop APIs such as rxSingle, rxMaybe, rxCompletable, etc. which internally leverage GlobalScope in order to break out of structured concurrency? Will these APIs be marked as delicate as well?

They don't really break out of structured concurrency, they encapsulated into the well-known world of cancellation in Rx. Supposedly, if you use them you are aware of the Rx-style approach to handling it.

The story is different with GlobalScope, since coroutines are structured by default, so bits of API that are not structured and require special attention need to be marked.

Copy link
Member

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

Awesome guide re-write, well done!

Few minor comments and it's good to go

docs/topics/coroutines-basics.md Outdated Show resolved Hide resolved
docs/topics/coroutines-basics.md Outdated Show resolved Hide resolved
@qwwdfsad qwwdfsad self-requested a review April 14, 2021 13:27
Copy link
Member

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

Also, please don't forget to add opt-in to our Gradle scripts, because now all the rx integrations is highlighted with warning

Copy link
Member

@koshachy koshachy left a comment

Choose a reason for hiding this comment

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

Some minor improvements.
I hope that soon we will be able to bring all the documentation to our guidelines.

docs/topics/coroutines-basics.md Outdated Show resolved Hide resolved
docs/topics/coroutines-basics.md Outdated Show resolved Hide resolved
docs/topics/coroutines-basics.md Outdated Show resolved Hide resolved
docs/topics/coroutines-basics.md Outdated Show resolved Hide resolved
docs/topics/coroutines-basics.md Outdated Show resolved Hide resolved
docs/topics/coroutines-basics.md Outdated Show resolved Hide resolved
docs/topics/coroutines-basics.md Outdated Show resolved Hide resolved
docs/topics/exception-handling.md Outdated Show resolved Hide resolved
docs/topics/composing-suspending-functions.md Outdated Show resolved Hide resolved
docs/topics/coroutine-context-and-dispatchers.md Outdated Show resolved Hide resolved
@mhernand40
Copy link

Also, please don't forget to add opt-in to our Gradle scripts, because now all the rx integrations is highlighted with warning

@elizarov, @qwwdfsad I guess this is what I was alluding to with my comment above. 🙂

elizarov and others added 15 commits April 15, 2021 16:58
Co-authored-by: Vsevolod Tolstopyatov <qwwdfsad@gmail.com>
Co-authored-by: Andrey Polyakov <koshachy@gmail.com>
Co-authored-by: Andrey Polyakov <koshachy@gmail.com>
Co-authored-by: Andrey Polyakov <koshachy@gmail.com>
Co-authored-by: Andrey Polyakov <koshachy@gmail.com>
Co-authored-by: Andrey Polyakov <koshachy@gmail.com>
Co-authored-by: Andrey Polyakov <koshachy@gmail.com>
Co-authored-by: Andrey Polyakov <koshachy@gmail.com>
Co-authored-by: Andrey Polyakov <koshachy@gmail.com>
Co-authored-by: Andrey Polyakov <koshachy@gmail.com>
Co-authored-by: Andrey Polyakov <koshachy@gmail.com>
Co-authored-by: Andrey Polyakov <koshachy@gmail.com>
@elizarov
Copy link
Contributor Author

Fixed review commend, also:

  • Added a formal definition on "what is a coroutine" to the basic docs (simplified version from the KEEP)
  • Added more documentation on the intended usage of CoroutineScope() function CoroutineScope in its docs, as well as mentioned it in GlobalScope docs.

@elizarov elizarov requested a review from qwwdfsad April 15, 2021 14:54
Copy link
Member

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

Good job!

@elizarov elizarov merged commit 6a42a77 into develop Apr 20, 2021
@elizarov elizarov deleted the global-scope-docs branch April 20, 2021 10:52
@whyoleg
Copy link
Contributor

whyoleg commented Apr 23, 2021

@elizarov may be NonCancellable also a good candidate for DelicateCoroutinesApi ?
reference to #1001 (comment). Until some other solution come in place?

@elizarov
Copy link
Contributor Author

IMO, NonCancellable is a different story. See GlobalScope is easy to abuse. People use it all the time when they should not be using it at all. I don't see this being a case with NonCancellable as much. Also, for NonCancellable we have a sketch of a different plan with an alternative.

@zach-klippenstein
Copy link
Contributor

This is awesome, thank you so much!

pablobaxter pushed a commit to pablobaxter/kotlinx.coroutines that referenced this pull request Sep 14, 2022
…hout it (Kotlin#2637)

Also, remove the tutorial variant of the basic coroutine introduction as it essentially duplicates the content of the
basic coroutine introduction, albeit with "how to create a project" additional information, which does not seem to be appropriate in this section at all.

Fixes Kotlin#2122

Co-authored-by: Vsevolod Tolstopyatov <qwwdfsad@gmail.com>
Co-authored-by: Andrey Polyakov <koshachy@gmail.com>
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.

None yet

6 participants