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

Shifting back to the default pool in Task.fromFuture, Task.async and … #921

Merged
merged 16 commits into from Jul 24, 2019

Conversation

@Avasil
Copy link
Collaborator

commented Jun 23, 2019

…others

Fixes #916

Should we have old behavior available? New method (unsafeXXX)? Flag? Deprecation? If I were just introducing these builders, I wouldn't include not-shifting variant so the API is safer but in our situation I am not so sure.

@Avasil Avasil requested review from alexandru and oleg-py Jun 23, 2019

@@ -42,14 +46,14 @@ private[eval] object TaskFromFuture {
rawAsync(startSimple(_, _, f))
}
case Some(value) =>
Task.fromTry(value)
Task.async(cb => cb(value))

This comment has been minimized.

Copy link
@Avasil

Avasil Jun 23, 2019

Author Collaborator

It sucks but I don't know if there is any way to detect whether Future has been built using successful or not

This comment has been minimized.

Copy link
@oleg-py

oleg-py Jun 23, 2019

Collaborator

Pretty sure those futures have .value set to Some(try)

This comment has been minimized.

Copy link
@Avasil

Avasil Jun 24, 2019

Author Collaborator

Future crossing async boundaries can also be Some(try) if it finished earlier

@Avasil

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 23, 2019

Oh, I didn't run monix-reactive tests. If we go with non-shifting versions I can rewrite them to use those, otherwise I'll fix tests

Avasil added 10 commits Jul 4, 2019
@oleg-py

This comment has been minimized.

Copy link
Collaborator

commented Jul 11, 2019

@Avasil @alexandru do you think it would make sense for Task.contextShift.evalOn to wrap EC into a Scheduler and use executeOn instead of bracketing two shifts?

It's pretty fragile as it is, and it seems more so with auto-shifting in builders that we're using in a typeclass instance.

@Avasil

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 11, 2019

@oleg-py I was wondering about it too and feel like it's the way to go.

@oleg-py
Copy link
Collaborator

left a comment

Not too happy with having so many builders, but I don't think there's a better option for now.

@Avasil

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 22, 2019

@oleg-py @alexandru
What do you think about the name createDirect, asyncDirect etc. instead of createUnsafe, asyncUnsafe ? "Direct" because we are calling Task callback directly and continue on the last thread directly

@oleg-py

This comment has been minimized.

Copy link
Collaborator

commented Jul 22, 2019

@Avasil honestly, probably a boolean flag with a default value and a consistent descriptive name on main builders (something like allowContinueOnCallingThread = false or shiftToMainPoolAfterResult = true) would be a better option. Otherwise, you still have to explain what you mean by direct, as you had to explain what unsafe meant.

Default parameters would probably make most sense here, as the method is still conceptually doing the same thing (lifting the asynchronous/cancelable operation into a Task) with a minor performance/thread-management option, and we don't have to worry much about eta-expansion issues it can cause due to it being FFI method that requires very explicit handling anyway.

Another option is to add an extra flag to Task.Options so you can executeWithOptions or set it in TaskApp and actually control from the outside if the shifting is going to happen. A positive side to it is that you can configure typeclass behavior this way. A negative one is that it's less intuitive, as we don't keep these two related things together - which might not be too bad for an option intended for power users.

@Avasil

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 22, 2019

@Avasil honestly, probably a boolean flag with a default value and a consistent descriptive name on main builders (something like allowContinueOnCallingThread = false or shiftToMainPoolAfterResult = true) would be a better option. Otherwise, you still have to explain what you mean by direct, as you had to explain what unsafe meant.

I will update a PR tomorrow, in my opinion flag fits quite nicely here. Also, if we change our minds it will be easier to deprecate it and introduce extra builders than in the other direction.

Another option is to add an extra flag to Task.Options so you can executeWithOptions or set it in TaskApp and actually control from the outside if the shifting is going to happen. A positive side to it is that you can configure typeclass behavior this way. A negative one is that it's less intuitive, as we don't keep these two related things together - which might not be too bad for an option intended for power users.

I always look at Task.Options as something to configure TaskRunLoop itself and here we deal with a builder. I also feel like those users who will use both variants may mix them a lot depending on the methods they invoke and that won't be very convenient

Avasil added 3 commits Jul 23, 2019
Merge branch 'master' into Task.create-shifts-back
# Conflicts:
#	project/MimaFilters.scala
@Avasil

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 23, 2019

@oleg-py Updated! If you feel like the old way was better then it's easy to revert

@oleg-py

This comment has been minimized.

Copy link
Collaborator

commented Jul 24, 2019

Nah, I think it's good. Not ambiguous like Unsafe, very explicit if you name boolean parameters, and arguably not less convenient to use than async-style builders already are.

@Avasil

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 24, 2019

Awesome! I'll just do a sanity check if I didn't miss anything important in a hurry and merge it

@Avasil Avasil merged commit fa8d0ad into monix:master Jul 24, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Avasil Avasil deleted the Avasil:Task.create-shifts-back branch Aug 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.