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 threading issue for sending values on subjects #15

Conversation

FranzBusch
Copy link
Contributor

There is a bug in the two subjects CurrentValueSubject and PassthroughSubject that the sending of new values is not synchronised. Right now this is only adding the tests to verify but I am working on implementing a fix as well, if you have input how it should be fixed please let me know.

@luoxiu
Copy link
Member

luoxiu commented Oct 12, 2019

Hi, @FranzBusch , I've created a pr to your branch and I think it will fix this, take a look when you have time!

@FranzBusch
Copy link
Contributor Author

@luoxiu The CI failed, these are the same failures that I saw locally when trying to fix it. It is either that the new test is green and the synchronous backpressure one is red or the other way round.

@luoxiu
Copy link
Member

luoxiu commented Oct 13, 2019

Ci succeeded on mac but failed on linux……

Hey, @FranzBusch , can you fix the conflicting issue? I've updated the travis's conf, let's start a new round of testing to see what will happen.

@FranzBusch
Copy link
Contributor Author

Sure will do!

@FranzBusch FranzBusch force-pushed the bugfix/subject-receive-value-synchronization branch from 1ea7a6a to 9a95018 Compare October 22, 2019 08:05
@@ -2,6 +2,14 @@ public final class Atom<Val> {

private let lock = Lock()
private var val: Val

var isMutating: Bool {
if lock.try() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build failed here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FranzBusch Your changes to the Lock was lost, :-D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah will fix it

@ddddxxx
Copy link
Member

ddddxxx commented Oct 22, 2019

@luoxiu We could probably add a new test target FailedTests, so that one can submit new test cases without breaking CI.

luoxiu added a commit that referenced this pull request Nov 4, 2019
@ddddxxx
Copy link
Member

ddddxxx commented Nov 4, 2019

Merged through #36. created #37. Thank you.

@ddddxxx ddddxxx closed this Nov 4, 2019
luoxiu added a commit that referenced this pull request Jun 1, 2021
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.

None yet

3 participants