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

Remove JoinHandle #83

Closed
wants to merge 1 commit into from
Closed

Remove JoinHandle #83

wants to merge 1 commit into from

Conversation

stbuehler
Copy link

Retrying #80 (also see #78). In this variant runtime::task::spawn returns no handle at all (but also only allows futures with () output).

For convenience a runtime::task::spawn_remote function is added which returns a RemoteHandle (from futures-rs).

It also applies #47 (removing runtime::spawn) as the API breaks anyway.

Motivation and Context

Removing the handle from the core API reduces overhead if you're not interested in the result.

If you are interested in the result you want a warning if you accidentally drop the handle, so RemoteHandle is the right choice.

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)

- spawn only works for Output=() now, so no handle for the result is
  needed (nor provided)
- spawn_remote returns a RemoteHandle, which aborts the future when
  dropped
- remove runtime::spawn reexport
@yoshuawuyts
Copy link
Collaborator

As outlined in #80, the changes in #78 are preferable over any other changes to the API. Thanks for your contributions, but this change is not in line with our current direction.

@stbuehler
Copy link
Author

As outlined in #80, the changes in #78 are preferable over any other changes to the API. Thanks for your contributions, but this change is not in line with our current direction.

I call bullshit. "the changes in #78 are preferable over any other changes to the API." - you made no such argument, and how would you even know if there isn't a change out there that might be better?

You listed two reasons, which I both fully disagree with and addressed in this PR anyway:

Given std::thread doesn't require calling join either that seems like a reasonable direction to take.

The new spawn doesn't require joining either.

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.

spawn_remote is just a convenience function and could easily depend on futures-util feature.


@aturon: given you are listed as the other lead in https://github.com/rustasync/team I'm including you here: I don't think @yoshuawuyts is up to the task maintaining this crate, and therefore consider forking it. I don't like going that way, but runtime seems to important to me right now to simply walk away.

@yoshuawuyts
Copy link
Collaborator

@stbuehler If you think this project is not heading into the direction you prefer, I definitely encourage you to fork it and make the changes you think are right! That's an intended mechanism of the licenses the Runtime crates are distributed under, and probably the most effective way of proving your ideas.

Aaron is currently on leave, and has generally moved onto managing different projects. Currently the best person besides me to direct your complaints about the direction this project is taking would be @stjepang. Or if you'd like to discuss my general aptitude to lead projects with other working group leads, I suggest reaching out to @bIgBV or @gruberb.

Also I'd like to share the way you've been behaving so far has been unprofessional to put it lightly. I do not take kindly to both insults and threats. The fact that you feel comfortable resorting to both when things don't go your way is enough of an indication to me that I do not wish to engage with you any further.

Thanks for your interest in Runtime; good luck on your future(s) ventures! I'm going to go ahead and lock this thread as it seems we've reached a logical end to our conversation.

@rustasync rustasync locked and limited conversation to collaborators Aug 10, 2019
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.

2 participants