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

bring std/channels back #20453

Closed
wants to merge 3 commits into from
Closed

bring std/channels back #20453

wants to merge 3 commits into from

Conversation

ringabout
Copy link
Member

No description provided.

@arnetheduck
Copy link
Contributor

Why put channels in std lib? channel libraries is one of those things where there are many tradeoffs and no clear winning implementation - it would be better to first see this developed outside of the std lib such that the implementation becomes stable and usable before putting the maintenance burden on the stdlib, with its increased backwards compatiblity requirements.

@planetis-m
Copy link
Contributor

I have a question I asked before but since I got no response, I will repeat here... Why is this implementation 'simplified' from the link on top of the file? For example ChannelCache is unused. And even the test example reports a data-race compiled with nim c --cc:clang --threadanalysis:off --tlsEmulation:off -d:useMalloc -t:"-fsanitize=thread" -l:"-fsanitize=thread" -d:nosignalhandler -d:release -g Is this benign?

@ringabout
Copy link
Member Author

ringabout commented Sep 29, 2022

Why is this implementation 'simplified' from the link on top of the file?

I'm not sure, changes are from nim-lang/threading#9

Why put channels in std lib?

The main concerns regarding std/channels from https://forum.nim-lang.org/t/7983#52533 are solved, so it is moved into stdlib again. v2 needs to ship with some thread primitives for ORC and it allows edge cases.

And even the test example reports a data-race compiled with nim c --cc:clang --threadanalysis:off --tlsEmulation:off -d:useMalloc -t:"-fsanitize=thread" -l:"-fsanitize=thread" -d:nosignalhandler -d:release -g

I will investigate it next week.

@arnetheduck
Copy link
Contributor

The main concerns regarding

The point is not so solve "the main concerns" - the point is to establish a library that actually works and that has a chance to stay that way - since ORC is a new memory model and a new threading model for which the rest of the standard library has not yet been adapted, adding threading primitives on top is not a good idea - you can't build a good house on top of an unstable base - the story of FlowVar and the previous channel / threading impls should teach this lesson: it's hard to get it right.

Adding it now, before there's a well-established use of it based on Nim 2.0 which yet doesn't work (ie simple applications can't be compiled with it) is a good way to add yet another sub-par module to the std lib that will contribute to a poor threading experience for a long time to come.

The appropriate time to start thinking about adding these primitives is maybe around 2.2, assuming there are some major users of the library at that point - if there are not, either the API is bad or there are more fundamental issues that need addressing before it can be used.

@arnetheduck
Copy link
Contributor

I'm not sure,

This is a problem in and of itself: maintaining a quality threading library requires expertise and deep understanding of what's being done - throwing things into the std lib without those things is a good way to end up with an unmaintained library that causes more problems than it solves, including security problems and copious amounts of bugs to add on to the already-full issue tracker.

@Araq
Copy link
Member

Araq commented Sep 29, 2022

Well you're right, @arnetheduck, but we think we got the API right this time and so all remaining bugs are implementation problems that can be fixed in 2.x etc. The real question for me is whether we should split Nim into compiler and stdlib repositories, both branching off from nim-lang/Nim in order to keep the git history.

@arnetheduck
Copy link
Contributor

but we think we got the API right this time

if that is the case, it will doubtless be proven by real world use cases and we can add it in 2.2.. 2.0 already has way too many difficult changes and it would be better to focus efforts on making the core std lib work well with those changes rather than adding new things that create more maintenance requirements.

Also, if 2.0 users start declaring which std lib parts they're using, that's a good thing: the explicit declaration provides the information needed to perform the split down the line: it's the missing piece to make a split compiler/stdlib-core/stdlib-other work (ie the tooling that puts the compiler and std lib parts together needs these declarations) - it's thus a feature that is part of that journey, and "testing" it on threading, an immature (from a 2.0 perspective) library is an excellent strategy for getting both things right.

@metagn
Copy link
Collaborator

metagn commented Sep 29, 2022

Can't comment on threading specifically as any time I've had to use it I've been fine with importing it as a package (it's not like I'm compiling on a microcontroller).

whether we should split Nim into compiler and stdlib repositories

The problem isn't just the stdlib having to be maintained with the compiler, it's unrelated modules in the stdlib having to be maintained with each other. I have no idea where stuff like this was originally discussed but it has been brought up recently a fair bit: nim-lang/RFCs#473, nim-lang/RFCs#476.

if users start declaring which std lib parts they're using, that's a good thing

I don't even think users would be bothered by this, see Rust crates and Java 9.

@Araq
Copy link
Member

Araq commented Sep 29, 2022

Closing for now, the discussion needs to happen elsewhere.

@Araq Araq closed this Sep 29, 2022
@ringabout ringabout deleted the pr_channels branch September 29, 2022 15:03
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.

6 participants