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

#52 Expand channel documentation #53

Merged
merged 2 commits into from
Dec 14, 2023

Conversation

PhilippMDoerner
Copy link
Contributor

Closes #52

A small PR based on what I learned about channels. Regard every change I made with suspicion, I am still starting out in learning about channels, multithreaded communication etc.

I very deliberately stole a lot of the descriptions from Nim's channels_builtin module (example ) and adjusted them to keep descriptions somewhat consistent.

I also added an external hyperlink to std/isolation to the general module docs, because the imports don't show in them and neither does Isolated have any hyperlinks to its own type. This makes it unclear for users where to find the type if they have no context (I mean, I had none, I'm just diving into the multithreading space now) and therefore there should be a link somewhere.

I am also very uncertain about a sentence I used repeatedly here:
## The memory src is moved, not copied.

That is my current understanding of threading/channels, but I don't quite know that for sure, so if that needs correcting please say so.

@PhilippMDoerner PhilippMDoerner marked this pull request as ready for review December 14, 2023 12:18
@Araq Araq added the merge once CI is green merge once CI is green label Dec 14, 2023
@Araq Araq merged commit 73e9dcc into nim-lang:master Dec 14, 2023
12 checks passed
@PhilippMDoerner PhilippMDoerner deleted the docs/#52-expand-channel-docs branch December 14, 2023 19:48
@ZoomRmc
Copy link
Contributor

ZoomRmc commented Dec 15, 2023

Sorry, I'm late to the party. Scanned through the changes and in general it's all good with an exception of tryRecv. Usually, "can fail for all sort of reasons" is not what you want to put in the documentation :P.

The fail case is single and known: an empty channel.

Moreover, "This returns immediately" is also not guaranteed, we're trying to acquire the lock unconditionally after all. May be we need to add the clarification that the procs don't block waiting for the data/space to appear in the channels, but block on lock contention. Just to concoct a scenario when it happens: no one prevents anyone from writing silly code and combining blocking and non-blocking versions so it's possible to put yourself in such a situation.

@PhilippMDoerner
Copy link
Contributor Author

PhilippMDoerner commented Dec 15, 2023

Happy to make a correction PR!

The failure bit I just took from builtin_channels.nim, the fact that tryRecv does not return immediately was actually not known to me, thanks for the explainer!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge once CI is green merge once CI is green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is trySend failing to send a message without returning false?
3 participants