Fix/mutable state bug #11
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mutable fields on `RWFocus` and `ROFocus` are shared between the
implementations of the `IDeref` and `IRef` interfaces
each interface tested in isolation is correct
the combination of the two is subtly incorrect in at least the
following sequence:
1) a watch is added to a derived atom
2) the derived atom is dereferenced
* this `set!`s the value `.-srccache`
3) the source atom changes, firing the `main-watcher`
* it sees the value of `.-srccache` from the `deref`...
* therefore it does not run the watchers on the derived atom, since
this test is true:
`(identical? newv (.-srccache self))`
this commit fixes the issue by splitting 3 independent mutable fields
for `srccache`, `oldcache`, and `cache`
I did not test if it is necessary to split all 3...
this change is sufficient to fix the bug, but it may also be overkill,
I am only certain that there is a problem with sharing `srccache`, not
the other two
|
First of all, thanks for your time. About the issue, after looking many times the code i don't see any relevant difference with the previous code:
Can you explain please which bug is fixing this, because i don't understad that is intend to solve. Maybe you first need write a test case that reproduces the bug? |
|
Yes, I'll be glad to put together a test case that shows the problem, but I will not be able to get to it right away. |
|
@blak3mill3r I think you forgot about this |
|
Closing this issue until someone can provide a reproducible test case or a coherent explanation of the problem. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
First, thank you for this library! I'm finding it useful.
There is a (very) subtle shared-state bug with the mutable cache fields which I fixed here
I don't have time to do this properly right now, though I would be happy to do so at some point... I didn't update the tests or even make sure that my changes were entirely necessary, but they do fix a nasty and subtle flaw which we ran into today using this library.