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

Disallow implicit blocking operators of Project Reactor's Flux #468

Closed
2 of 4 tasks
Ernir opened this issue Jan 18, 2023 · 2 comments · Fixed by #472
Closed
2 of 4 tasks

Disallow implicit blocking operators of Project Reactor's Flux #468

Ernir opened this issue Jan 18, 2023 · 2 comments · Fixed by #472
Labels
Milestone

Comments

@Ernir
Copy link

Ernir commented Jan 18, 2023

Problem

Excessive blocking is a known pitfall in reactive programming. Some of Reactor's blocking operators are quite explicit about their behavior, e.g. Mono#block and Flux#blockFirst, leaving no doubt that the operation is in fact blocking.

This does not apply to two of the convenience methods exposed by Reactor's Flux, Flux#toStream and Flux#toIterable. These operators are documented to block, but this is not apparent from the method signature. It is easy to miss that the operation is blocking despite any potential lazy consumption of the resulting Stream or Iterable.

Description of the proposed new feature

  • Support a stylistic preference.
  • Avoid a common gotcha, or potential problem.
  • Improve performance.

We should rewrite the problematic operators in terms of explicitly blocking operators, highlighting actual behavior.

Focusing first on the stream, I would like to rewrite the following code:

flux.toStream()

to:

flux.collect(toList()).block().stream()

To be determined what a general rewrite for Flux#toIterable would look like, or if this is actually desirable.

Considerations

  • The methods in question have overloads detailing their usage. We could consider allowing those, assuming that those operators are less likely to be used accidentally. Compare with the existing FluxFlatMapUsage rule.
  • The problematic methods being convenience methods, the proposed solution is more verbose than the original.

Participation

  • I am willing to submit a pull request to implement this improvement.

Willing, sure! Less sure on able. Would be happy to see anyone else submit a pull request to this effect. :D

@rickie rickie added the good first issue Good for newcomers label Jan 19, 2023
benhalasi added a commit to benhalasi/error-prone-support that referenced this issue Jan 20, 2023
@Stephan202
Copy link
Member

See #472 (comment) for some discussion on the approach to take here.

benhalasi added a commit to benhalasi/error-prone-support that referenced this issue Jan 23, 2023
rickie pushed a commit to benhalasi/error-prone-support that referenced this issue Jan 25, 2023
rickie pushed a commit to benhalasi/error-prone-support that referenced this issue Jan 25, 2023
benhalasi added a commit to benhalasi/error-prone-support that referenced this issue Jan 29, 2023
benhalasi added a commit to benhalasi/error-prone-support that referenced this issue Jan 29, 2023
benhalasi added a commit to benhalasi/error-prone-support that referenced this issue Jan 31, 2023
benhalasi added a commit to benhalasi/error-prone-support that referenced this issue Jan 31, 2023
rickie pushed a commit to benhalasi/error-prone-support that referenced this issue Feb 8, 2023
rickie pushed a commit to benhalasi/error-prone-support that referenced this issue Feb 8, 2023
Stephan202 pushed a commit to benhalasi/error-prone-support that referenced this issue Feb 18, 2023
Stephan202 pushed a commit to benhalasi/error-prone-support that referenced this issue Feb 18, 2023
rickie pushed a commit to benhalasi/error-prone-support that referenced this issue Feb 20, 2023
rickie pushed a commit to benhalasi/error-prone-support that referenced this issue Feb 20, 2023
@Ernir
Copy link
Author

Ernir commented Feb 20, 2023

Thanks @benhalasi , @rickie , @Stephan202 !

@Stephan202 Stephan202 added this to the 0.9.0 milestone Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

3 participants