fix: ensure Topic#publish1 returns accurate result during concurrent closure#3665
fix: ensure Topic#publish1 returns accurate result during concurrent closure#3665guptapratykshh wants to merge 5 commits intotypelevel:mainfrom
Conversation
|
@mpilquist can you have a look at this PR , Thanks. |
|
Need to rebase this PR now that other PRs have been merged. Also, need to remove the |
guptapratykshh
force-pushed
the
fix/issue-3644-topic-publish-race
branch
2 times, most recently
from
da8bc03 to
bd2981e
Compare
guptapratykshh
force-pushed
the
fix/issue-3644-topic-publish-race
branch
from
bd2981e to
a30644f
Compare
|
@TomasMikula Mind reviewing this PR? Thanks! |
|
So this PR guarantees that
whereas the failing test also requires that
I admit the test is somewhat strict, and I'd consider the implemented behavior acceptable if clearly documented that return value |
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Minor thing: I think constructing the effect in a right-associative way will lead to slightly faster execution for an IO-like F.
|
@guptapratykshh So you went full on to satisfy the test as is (as opposed to relaxing the test as suggested above). My main concern with this implementation is that
These might be breaking changes for existing users. |
TomasMikula
left a comment
TomasMikula
left a comment
There was a problem hiding this comment.
Looks good to me, except the one (non-essential) nitpick.
| case State.Active(subs, _) => | ||
| foreach(subs)(_.send(a).void) | ||
| .as(Topic.rightUnit) | ||
| subs.foldLeft(Topic.rightUnit.pure[F]: F[Either[Topic.Closed, Unit]]) { |
There was a problem hiding this comment.
This might be a tiny bit simpler using foldLeftM. What do you think?
There was a problem hiding this comment.
i have refactored publish1 to use foldLeftM
guptapratykshh
force-pushed
the
fix/issue-3644-topic-publish-race
branch
from
464c7cd to
eb30b4e
Compare
3 participants
this pr ensures Topic#publish1 accurately returns Left(Closed) when delivery fails due to concurrent topic closure by checking individual subscriber channel results and also guarantees that Right(()) response from publish1 consistently signifies successful delivery to all currently registered subscribers.
Resolves #3644