Fix freezes caused by concurrently produced iterators #4416
Conversation
|
I'm not marking this PR as draft because it is basically done and ready. The only thing I'm not sure about is the new role name. Is Also, when the role is moved into a dedicated file it's gonna be almost the first case of adding a new one since Raku extensions were approved. So, it's tempting to make it the first |
`HyperSeq` and `RaceSeq` should throw if an iterator has been already produced for them, similar to what `Seq` currently does. In support of rakudo/rakudo#4416 and rakudo/rakudo#4413
`HyperSeq` and `RaceSeq` should throw if an iterator has been already produced for them, similar to what `Seq` currently does. In support of rakudo/rakudo#4416 and rakudo/rakudo#4413
|
As this is mostly @jnthn's brainchild, I'm deferring review to him. I would definitely not merge this for a 2021.06 release. On other notes:
|
|
Given |
src/core.c/Exception.pm6
Outdated
| "by assigning the Seq into an array)" | ||
| ("The iterator of this $!kind is already in use/consumed by another $!kind" ~ | ||
| " (you might solve this by adding .cache on usages of the $!kind, or " ~ | ||
| "by assigning the $!kind into an array)").naive-word-wrapper |
There was a problem hiding this comment.
Hm what is naive-word-wrapper? I'd expect such things to be applied at the point of exception printing, not in the message production...
There was a problem hiding this comment.
But we currently don't do it anyway. There are a couple other exceptions doing this. This particular message was previously using newlines inside the message to wrap it. So, I'd keep it this way until the printing starts wrapping.
There was a problem hiding this comment.
Yes, to be fair this was a more general grumble rather than anything that should block this PR.
Take into account @jnthn notes from rakudo#4416 (rakudo#4416): - Rename `Sequence::Hyper` into `ParallelSequence` - Extract `ParallelSequence` into own file ParallelSequence.rakumod - Make `X::Seq::Consumed` expect `$.kind` to contain a sequence class - Make `ParallelSequence` `iterator` method thread-safe by using a `cas`-set flag. - Add `ParallelSequence` to the list of expected symbol in tests
|
@jnthn All notes were taken into account except for the I also considered making |
vrurg
changed the title
| my role ParallelSequence does Iterable does Sequence { | ||
| has HyperConfiguration $.configuration; | ||
| has Rakudo::Internals::HyperWorkStage $!work-stage-head; | ||
| has $!has-iterator; |
There was a problem hiding this comment.
I think this should be has atomicint $!has-iterator
There was a problem hiding this comment.
(What you have relies on interning of Int, which is probably reliable, but still...)
There was a problem hiding this comment.
I think this should be
has atomicint $!has-iterator
atomicint was my choice too, but it is not generally available at this point. I tried changing the order of build with no success. But there was another error which I located later. Perhaps it was the problem, not the order. So, I will try that later today.
Freezes were happening with code similar to:
(.list, .list) given (^10).hyper;
The cause was tracked down to two iterators created for the same
`$!work-stage-head` object. In an identical situation `Seq` would throw
`X::Seq::Consumed` which seems logical. Similar approach was taken for
`HyperSeq` and `RaceSeq`.
Aside of this, I noticed that `HyperSeq` and `RaceSeq` share basicall
all their code except for `hyper` and `race` methods. I found it
reasonable to move all commonalities into a role.
Fixes rakudo#4413
Take into account @jnthn notes from rakudo#4416 (rakudo#4416): - Rename `Sequence::Hyper` into `ParallelSequence` - Extract `ParallelSequence` into own file ParallelSequence.rakumod - Make `X::Seq::Consumed` expect `$.kind` to contain a sequence class - Make `ParallelSequence` `iterator` method thread-safe by using a `cas`-set flag. - Add `ParallelSequence` to the list of expected symbol in tests
`HyperSeq` and `RaceSeq` should throw if an iterator has been already produced for them, similar to what `Seq` currently does. In support of rakudo/rakudo#4416 and rakudo/rakudo#4413
This required atomicops.pm6 to be moved upper in the order of core files.
|
I will merge when get JVM backend fixed. It would probably have to deal with |
JVM doesn't have `atomicint`.
Freezes were happening with code similar to:
The cause was tracked down to two iterators created for the same
$!work-stage-headobject. In an identical situationSeqwould throwX::Seq::Consumedwhich seems logical. Similar approach was taken forHyperSeqandRaceSeq.Aside of this, I noticed that
HyperSeqandRaceSeqshare basicallall their code except for
hyperandracemethods. I found itreasonable to move all commonalities into a role.
Fixes #4413