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

fix false must_use warning on JoinHandle #78

Conversation

rschmukler
Copy link
Contributor

@rschmukler rschmukler commented Aug 2, 2019

Removes the must_use requirement on the JoinHandle

Motivation and Context

The must_use requirement is a bit misleading, since the JoinHandle is the
result of a future being spawned, and thus the originating future will be
polled to completion, whether or not the JoinHandle is polled.

This seems like it may overlap with some of the changes in #73, where it may be desirable to instead use a RemoteHandle - but since there is some open discussion, this seems like a worthy (and simple) change.

Another option might be to add a .forget() method which consumes the JoinHandle directly, and is thus a bit more explicit.

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)

Removes the must_use requirement on the JoinHandle, since it is the
result of a future being spawned - thus, the originating future will be
polled to completion, whether or not the JoinHandle is polled.
@rschmukler rschmukler force-pushed the rschmukler/remove-join-handle-must-use branch from bd8df31 to c290baf Compare August 2, 2019 20:51
@rschmukler rschmukler changed the title fix false muse_use warning on JoinHandle fix false must_use warning on JoinHandle Aug 2, 2019
@stbuehler
Copy link

I'd recommend replacing it with RemoteHandle directly; this would change the semantic a little (RemoteHandle must actually be used; if dropped without calling forget it will cancel the future) - basically the opposite direction of this pull request.

@stbuehler
Copy link

I.e. I'd suggest this: master...stbuehler:replace-joinhandle

@rschmukler
Copy link
Contributor Author

@stbuehler I'm not opposed to that direction at all, but it is worth noting that that behavior is different than how existing runtimes (tokio, namely) currently operate. Do you want to open a PR and perhaps let the maintainers decide which way they want to go? I'm mostly indifferent.

@stbuehler stbuehler mentioned this pull request Aug 4, 2019
3 tasks
@stbuehler
Copy link

@rschmukler Done. I'm curious though which part in tokio you mean - spawn in tokio doesn't actually return something useful right now.

I think futures would be the more appropriate place to make this decision - I don't care much which way it goes either, but I think using types from futures is the right way.

@rschmukler
Copy link
Contributor Author

rschmukler commented Aug 4, 2019

@stbuehler Sorry, I actually remembered the incorrect behavior. Your change is actually very consistent with the futures::sync::oneshot::spawn (0.1.21) API (which has a SpawnHandle that behaves exactly as a RemoteHandle). My inclination is to emulate that behavior since it seems least surprising - making your change, in my book, the preferred route to take. I'll close this, although the maintainers are welcome to take it if they want it.

@rschmukler rschmukler closed this Aug 4, 2019
@rschmukler rschmukler reopened this Aug 5, 2019
@rschmukler
Copy link
Contributor Author

Reopening per the discussion in #80

@stbuehler stbuehler mentioned this pull request Aug 10, 2019
3 tasks
Copy link
Collaborator

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

Thanks heaps! We'll make this part of the next Runtime release!

@yoshuawuyts yoshuawuyts merged commit 19a1a82 into rustasync:master 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.

3 participants