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

Cancel existing operation in a watcher before starting a new one #1012

Merged
merged 1 commit into from
Feb 12, 2020

Conversation

designatednerd
Copy link
Contributor

Thanks to @RolandasRazma for the suggestion in #1011 - this removes the hack needed to make things pass added in #1010.

There's still some pretty significant thread safety issues going on here as pointed out by @cysp, but I think there's a BIG rabbit hole there, and I'd at least like to remove the dirty hack ASAP.

@designatednerd designatednerd added this to the 0.22.0 milestone Feb 12, 2020
Copy link
Contributor

@cysp cysp left a comment

Choose a reason for hiding this comment

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

Cancelling any previous fetch definitely seems like something that should be done.

I guess no part of the test suite asserts the value of that state property?

@designatednerd
Copy link
Contributor Author

@cysp It doesn't appear that we have tests specifically for either AsynchronousOperation or for FetchQueryOperation, which seems to be where the main issue is. We also don't have TSAN turned on for tests in general because there's a bunch of tangled stuff causing races we haven't (so far) been able to detangle. Again, any help we can get on that would be much appreciated.

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