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
Creating race conditions when converting akka streams to fs2 streams is too easy #54
Comments
I like the general idea of exposing the materialized value as FS2 stream instead of providing a callback (which makes composition on FS2 side hard or impossible). But for better symmetry with Akka Sink|Source|Flow plus materialized value what do you think about adding converters like
This would allow applications to compose the result tuple elements as they like in FS2. Also, when converting an Akka Source to an FS2 Stream, the Stream element type and the materialized value type usually differ and cannot be combined into a single Stream (unless you use something like The |
That's not a bad idea - we can have |
Sounds good, would be great to see a PR! I'm not sure if |
I don't think it's too bad - If there's a used |
I have a WIP proposal here: master...Daenyth:matvalue I'm playing around a bit to see if I can keep the single method name next, while still having the differing return types based on |
Understood. If discarding can be done explicitly than it's fine. I thought users are forced to use |
The problem:
Akka streams have materialized values, but the default
.toStream()
or.toSink()
discard them. I've created race conditions by converting sinks with a materialized Future result and not waiting on that future.Ad-hoc solution:
I created this:
The problem with my approach is that the user is still required to realize they need the alternate converter method.
A more comprehensive solution:
I'd like to make a breaking API change before we make the 1.x release.
Proposed:
This guarantees that a caller must either provide an explicit
onMaterialization
, for whichDiscard
will be wildcard imported for convenience, or if they don't want to pass one, thatM
must beNotUsed
.I haven't tested this yet, but I believe the general approach should work.
If you're willing to take this, I'll send a PR implementing it
The text was updated successfully, but these errors were encountered: