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

Add Stream#compileInterruptible to guarantee use of Concurrent compiler #2678

Closed
wants to merge 1 commit into from

Conversation

bplommer
Copy link
Contributor

No description provided.

@bplommer bplommer marked this pull request as ready for review October 13, 2021 08:27
@bplommer
Copy link
Contributor Author

Another option would be a no-op .interruptible on CompileOps 🤔

@mpilquist
Copy link
Member

Of the two options, I prefer .compile.interruptible. I lean towards not adding either though. Folks would need to get in the habit of always using .compile.interruptible in order for it to be effective in finding cases where they were compiling with Sync. If we're really concerned about that case, then I think we should dust off the approach in #2650.

@bplommer
Copy link
Contributor Author

bplommer commented Oct 13, 2021

Folks would need to get in the habit of always using .compile.interruptible in order for it to be effective in finding cases where they were compiling with Sync.

I see this as both an upside and a downside - adding this doesn't automatically provide any extra safety, but it lets users decide if they're concerned enough to opt in to it. (And doing so just requires a simple find/replace). It seems at least worth providing that option.

If we're really concerned about that case, then I think we should dust off the approach in #2650.

I lean in favour of that approach but it comes at the cost if inconveniencing users - it could be made more ergonomic, though, by having a separate compile method (compileSync?) rather than requiring manually providing an instance.

@mpilquist
Copy link
Member

That's an interesting idea. If we go that route, I'd try to convince doobie to add an implicit Compiler.Target[ConnectionIO] instance but compileSync could exist to help cases where folks really want that behavior.

Now that we've merged #2657, can we come up with some sample expressions which exhibit unexpected / undesirable behavior? I'm curious if there are any left.

@bplommer
Copy link
Contributor Author

That's an interesting idea. If we go that route, I'd try to convince doobie to add an implicit Compiler.Target[ConnectionIO] instance but compileSync could exist to help cases where folks really want that behavior.

Wouldn't that just result in compile producing the same behaviour as compileSync?

@mpilquist
Copy link
Member

Yep. .compileSync would only be needed when library authors haven't opted in to a sync compilation by providing such an instance.

@bplommer
Copy link
Contributor Author

bplommer commented Nov 6, 2021

Another approach is to use a scalafix rule to lint for use of the sync compiler - we did this for http4s in http4s/http4s#5536

@Daenyth
Copy link
Contributor

Daenyth commented Nov 30, 2021

Folks would need to get in the habit of always using .compile.interruptible in order for it to be effective in finding cases where they were compiling with Sync

This is a big problem for me because it raises the learning curve of the library and it provides another point where a scala dev is forced to make a decision that both 1) requires them to have a very firm grasp of library details (this is hard for onboarding) and 2) the result of the decision in many cases just doesn't matter

I'm not a fan of adding this method.

@mpilquist
Copy link
Member

I think we have rough consensus to not add this API at this point. Closing this for now but let's reopen if we learn more -- in particular, if we find any examples with surprising behavior as a result of using a sync constraint.

@mpilquist mpilquist closed this Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants