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

Do we need to support invoking sync functions? #77

Closed
goodboy opened this issue May 7, 2019 · 5 comments · Fixed by #205
Closed

Do we need to support invoking sync functions? #77

goodboy opened this issue May 7, 2019 · 5 comments · Fixed by #205
Assignees
Labels
Milestone

Comments

@goodboy
Copy link
Owner

goodboy commented May 7, 2019

@jtrakk made a good point in gitter that supporting both async and regular functions for remote invocation is a bit odd and probably unnecessary. I think I agree; there ain't nothing a regular func can do that an async one can't. Unless I'm forgetting something I think it should be fine to just remove support for regular functions?

Off hand it would allow removing the slew of checks in _actor._invoke().

@ryanhiebert
Copy link
Collaborator

I'd be in favor of only supporting async functions, for exactly the reasons you describe. I don't think there's anything significant to be gained, but there is additional complexity that could easily be avoided.

@goodboy goodboy added this to the 0.1.0.a0 milestone Oct 22, 2019
@goodboy goodboy modified the milestones: 0.0.0.alpha0, 0.0.0a0.dev0 Sep 27, 2020
@goodboy goodboy changed the title Is there a reason to support invoking regular functions? Do we need to support invoking sync functions? Jan 31, 2021
@goodboy
Copy link
Owner Author

goodboy commented Jan 31, 2021

See the comment in richardsheridan/trio-parallel#2 which I think is more fuel for this change.

@goodboy
Copy link
Owner Author

goodboy commented Feb 25, 2021

Yah, I'm thinking for a .alpha0 release we just drop remote sync function calling support and defer to @richardsheridan's project.

@goodboy
Copy link
Owner Author

goodboy commented Feb 25, 2021

Yeah so TLDR: is that if we're calling sync funcs in some remote actor you'll need the tractor runtime anyway which means you should have the ability to wrap the call in an async func - so there should be no benefit to supporting that wrapping inside _actor._invoke.

I also think we should make a alt-projects list in the readme and point to others for this kind of thing.

@goodboy goodboy modified the milestones: 0.0.0a0.dev0, 0.0.0.alpha0 Feb 25, 2021
@goodboy
Copy link
Owner Author

goodboy commented Apr 26, 2021

We already have a pointer to trio-parallel in the readme, so just need a PR removing the support now.

@goodboy goodboy self-assigned this Apr 26, 2021
goodboy added a commit that referenced this issue Apr 27, 2021
You can always wrap a sync function in an async one and there seems to
be no good reason to support invoking them directly especially since
cancellation won't work without some thread hackery. If it's requested
we'll point users to `trio-parallel`.

Resolves #77
goodboy added a commit that referenced this issue Apr 27, 2021
You can always wrap a sync function in an async one and there seems to
be no good reason to support invoking them directly especially since
cancellation won't work without some thread hackery. If it's requested
we'll point users to `trio-parallel`.

Resolves #77
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants