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

fix: remove sampleStart and sampleEnd comments from example in coroutine-context-and-dispatchers.md #4081

Merged
merged 2 commits into from
Apr 3, 2024

Conversation

PetrakovichVictoria
Copy link
Contributor

@PetrakovichVictoria PetrakovichVictoria commented Apr 2, 2024

https://kotlinlang.org/docs/coroutine-context-and-dispatchers.html#jumping-between-threads

UPD: removed sampleStart and sampleEnd comments.

Fix this sample not being runnable:
Screenshot 2024-04-02 at 13 00 09

@dkhalanskyjb
Copy link
Collaborator

This example isn't running correctly, but fails with

Exception in thread "main" java.security.AccessControlException: Access control exception due to security reasons in web playground: 
 access denied ("java.lang.RuntimePermission" "modifyThread")
 at java.lang.SecurityManager.checkPermission (:-1) 

@PetrakovichVictoria
Copy link
Contributor Author

I don't know if the problem is in the coroutines here or in some restrictions in the playground. Maybe it's just worth deleting //sampleStart and sampleEnd comments and making this example non-runnable.

@dkhalanskyjb
Copy link
Collaborator

This example is being run as part of our tests (https://github.com/Kotlin/kotlinx.coroutines/blob/master/kotlinx-coroutines-core/jvm/test/guide/test/DispatcherGuideTest.kt#L37-L44); so, these are probably just the playground restrictions. Also, making the example non-runnable would remove the test automatically, and I don't think we want that.

@qwwdfsad
Copy link
Collaborator

qwwdfsad commented Apr 3, 2024

It's https://youtrack.jetbrains.com/issue/KTL-108 unfortunately

@PetrakovichVictoria
Copy link
Contributor Author

This example is being run as part of our tests

I also launched it before I created this PR: locally, this example passes w/o problems.

Also, making the example non-runnable would remove the test automatically, and I don't think we want that.

  1. Having sampleStart and sampleEnd shows an initial will for this example to be runnable.
  2. Almost all other examples from this page work (except here because these ones show some thought in general). So, having this one example non-runnable seems strange twice.
  3. For me, it's not that much important will it be runnable or not, but I suppose, if we want to leave it non-runnable, then //sampleStart and //sampleEnd should be removed (and the runnability marker too). I can do this if you wish.

@dkhalanskyjb
Copy link
Collaborator

if we want to leave it non-runnable, then //sampleStart and //sampleEnd should be removed (and the runnability marker too).

Won't that also remove the test generated by Knit?

…e comments about sample start and sample end
@PetrakovichVictoria
Copy link
Contributor Author

I don't see any changes after running knit locally. Removed runnability and sample start/end comments.

@PetrakovichVictoria PetrakovichVictoria changed the title fix: make code sample runnable in Jumping between threads in coroutine-context-and-dispatchers.md fix: remove sampleStart and sampleEnd comments in coroutine-context-and-dispatchers.md Apr 3, 2024
@PetrakovichVictoria PetrakovichVictoria changed the title fix: remove sampleStart and sampleEnd comments in coroutine-context-and-dispatchers.md fix: remove sampleStart and sampleEnd comments from example in coroutine-context-and-dispatchers.md Apr 3, 2024
@dkhalanskyjb dkhalanskyjb merged commit 20707d3 into master Apr 3, 2024
1 check passed
@dkhalanskyjb dkhalanskyjb deleted the fix-make-sample-runnable branch April 3, 2024 14:09
@dkhalanskyjb
Copy link
Collaborator

Thanks!

knisht pushed a commit to JetBrains/intellij-deps-kotlinx.coroutines that referenced this pull request Apr 15, 2024
mikegr pushed a commit to mikegr/kotlinx.coroutines that referenced this pull request Apr 17, 2024
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.

4 participants