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

API additions for connecting non-Send closures and thread-safety fixes to the main context channel and futures #541

Merged
merged 8 commits into from Nov 19, 2019

Conversation

@sdroege
Copy link
Member

sdroege commented Nov 18, 2019

After checking that the callback of the main context channel was always dropped from the correct thread, this check was actually failing often. I refactored the code now so that this can't happen anymore and we always ensure that non-Send values are only ever accessed from the correct thread.

Similar things happened with the GSource in the futures executor, which is also fixed and refactored now.

Overall the code should be much simpler now, and more correct!

sdroege added 8 commits Nov 18, 2019
… from a Sender

Otherwise we would unref it potentially from another thread and then
cause the callback of the source to be dropped from a different thread
than where it was created. This would be wrong as the callback closure
is not Send!

Do this by counting the number of senders separately instead of re-using
the strong count of the channel Arc.
These are not needed if all the signalling is happening by setting the
ready time of the GSource.
…opped from a different thread

The TaskSource now only keeps the Future that has to be polled and a
reference to a Waker, which is a child source of the TaskSource.

This Waker is passed as waker to the future's poll() function and is
potentially passed to other threads and might also outlive the
TaskSource but its only job is to wake up the TaskSource.

The TaskSource is never passed to different threads and is internal
to the futures executor.

Also remove the check/prepare functions from the GSources as they're not
needed when using child sources or triggering the GSources via setting
the ready time.
@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Nov 18, 2019

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Nov 18, 2019

Code looks good, thanks a lot! Now we need tests. :)

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Nov 18, 2019

Code looks good, thanks a lot! Now we need tests. :)

We already have tests. The bugs were caught by the existing tests after adding the checks for doing things from the correct thread :)

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

GuillaumeGomez commented Nov 18, 2019

If the CI was ok before, then it means it's not checked correctly. Please either update the travis build or add tests. :D

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Nov 18, 2019

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Nov 19, 2019

@sdroege Thanks.
There was one job failed due network, I restarted it

@sdroege

This comment has been minimized.

Copy link
Member Author

sdroege commented Nov 19, 2019

There was one job failed due network, I restarted it

Thanks, let's get it in then!

@sdroege sdroege merged commit ef63da4 into gtk-rs:master Nov 19, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.