Skip to content
This repository has been archived by the owner on Oct 30, 2019. It is now read-only.

Replace JoinHandle with RemoteHandle #80

Closed
wants to merge 2 commits into from
Closed

Replace JoinHandle with RemoteHandle #80

wants to merge 2 commits into from

Conversation

stbuehler
Copy link

Replace JoinHandle with RemoteHandle from futures.

This is a counter proposal to #78.

It changes the semantic a little (RemoteHandle must actually be used; if dropped without calling forget it will cancel the future) - basically the opposite direction of #78.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@@ -113,38 +114,15 @@ mod baseline {
}

/// Spawn function for juliex to get back a handle
pub fn spawn<F, T>(fut: F) -> JoinHandle<T>
pub fn spawn<F, T>(fut: F) -> RemoteHandle<T>
Copy link
Collaborator

Choose a reason for hiding this comment

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

The model we've been using so far is Tasks being async versions of Threads. In so the naming of JoinHandle would be more accurate, as it has a direct counterpart in std::thread::JoinHandle.

@yoshuawuyts
Copy link
Collaborator

yoshuawuyts commented Aug 5, 2019

The problem outlined in #78 is that the #[must_use] message is not always correct. Given std::thread doesn't require calling join either that seems like a reasonable direction to take.

This patch would increase our dependency on futures-rs's extensions, where we'd likely want to move to futures-core once that's released instead. Overall I'd prefer to land #78, as that seems a better fit for the direction we're heading. Thanks!

@stbuehler
Copy link
Author

You really don't know what you want. You are scared about atomic memory ordering, and you still want to maintain functionality needing just that which is (almost) already present in a "base" library to "reduce depedencies".

Imho: if you really want the JoinHandle semantics, just ask the futures-rs guys to add it. This is an obvious example of functionality that should be provided by futures(-utils), and not be implemented on your own.

The idea about having different futures-* crates is not that you don't use them, but that it is easier to bump the version outside of the futures-core crate without breaking interopability.

As I said in #78 (comment):

I don't care much which way it goes either, but I think using types from futures is the right way.

@yoshuawuyts
Copy link
Collaborator

yoshuawuyts commented Aug 5, 2019

@stbuehler I sense you're frustrated, but really I'm in no mood to deal with the way you're engaging here.

Closing this PR because it's not a change we desire at the time.

@yoshuawuyts yoshuawuyts closed this Aug 5, 2019
@rschmukler
Copy link
Contributor

@yoshuawuyts per your comments in here, I've reopened #78. Thanks!

@stbuehler stbuehler mentioned this pull request Aug 10, 2019
3 tasks
@stbuehler stbuehler deleted the replace-joinhandle branch August 10, 2019 10:38
@stbuehler
Copy link
Author

Yes, I'm frustrated.

I don't enjoy dealing with stupid maintainer decisions. Reducing dependency to a high quality core crate that 99% of your users are going to need anyway and reimplementing its functionality which uses features you're not comfortable to review is just plain stupid. Deal with it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants