Skip to content

/spring-framework

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

Jump to bottom

Fix potential leaks from Mono.just(dataBuffer) or Flux.just(dataBuffer) [SPR-17575] #22107

Closed
spring-issuemaster opened this issue Dec 6, 2018 · 3 comments
Assignees
@rstoyanchev
Labels
in: web type: task
Milestone
5.1.6

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Dec 6, 2018 •
edited by rstoyanchev

Rossen Stoyanchev opened SPR-17575

When using Mono.just or Flux.just data buffers can be allocated before subscription and consequently won't even see a cancellation signal in case the connection is closed.

The second issue is that if Mono.just or Flux.just is passed into response#writeWith those will not see a cancellation signal because cancellation is not propagated to them (i.e. can't cancel what's already been materialized).

Affects: 5.1.3
@spring-issuemaster spring-issuemaster added this to the 5.1.5 milestone Jan 11, 2019
@rstoyanchev

This comment has been minimized.

Sign in to view
Copy link
Contributor

@rstoyanchev rstoyanchev commented Feb 12, 2019

We do have a few instances of Mono.just or Flux.just with an allocated DataBuffer, e.g. in form writer, multipart writer, EncoderHttpMessageWriter, Jaxb2 encoder. While in the SSE writer the same is wrapped with Mono.defer.

Rescheduling for 5.1.6 to confirm those are issues and confirm the preferred fix, e.g. via defer? There is probably a small chance for the things that could go wrong before those are written. Nevertheless we should address it.
@rstoyanchev rstoyanchev modified the milestones: 5.1.5, 5.1.6 Feb 12, 2019
@poutsma

This comment has been minimized.

Sign in to view
Copy link
Contributor

@poutsma poutsma commented Feb 14, 2019

We do have a few instances of Mono.just or Flux.just with an allocated DataBuffer

most of these examples do not have any memory leaks:

  • the form writer decorates the ByteBuffer into a DataBuffer using DataBufferFactory.wrap(), which does not allocate.
  • multipart writer does indeed appear to leak: all generate* methods should probably be returning Mono<DataBuffer> and use Mono.defer.
  • EncoderHttpMessageWriter does not allocate buffers, so there is no need to release.
  • Jaxb2 encoder has a boolean release flag that is used in the finally block.
@rstoyanchev

This comment has been minimized.

Sign in to view
Copy link
Contributor

@rstoyanchev rstoyanchev commented Mar 25, 2019 •
edited

EncoderHttpMessageWriter does not allocate buffers, so there is no need to release.

EncoderHttpMessageWriter itself does not but the encoder it delegates to does, and the use of flatMap will prefetch eagerly and would cache before it reaches Reactor Netty.

Jaxb2 encoder has a boolean release flag that is used in the finally block.

The boolean releases in case of an error. The scenario being discussed in this ticket is that the DataBuffer is produced eagerly before subscription, and a cancellation may occur before it is written.
@rstoyanchev rstoyanchev changed the title Review code that sends pre-created DataBuffer's wrapped with Mono or Flux.just [SPR-17575] Fix potential leaks from Mono.just(dataBuffer) or Flux.just(dataBuffer) [SPR-17575] Mar 26, 2019
rstoyanchev added a commit that referenced this issue Mar 28, 2019
@rstoyanchev
Uncomment buffer leak tests in DataBufferUtils
beae1fb 
Following a response on
reactor/reactor-core#1634 apparently
FluxSink is respecting doOnDiscard but there is a race condition
in DataBufferUtils with file reading.

This commit makes two changes:
1) Add more checks for onDispose to detect cancellation signals
as well as to deal with potentially concurrent such signal.
2) Do not close the channel through the Flux.using callback but
rather allow the current I/O callback to take place and only then
close the channel or else the buffer is left hanging.

Despite this tests still can fail due to a suspected issue in Reactor
itself with the doOnDiscard callback for FluxSink. That's tracked under
the same issue reactor/reactor-core#1634
and for now the use of DefaultDataBufferFactory is enforced as a
workaround until the issue is resolved.

See gh-22107
rstoyanchev added a commit that referenced this issue Mar 29, 2019
@rstoyanchev
Call onDispose before first read
81f95ef 
The cancellation callback in asynchronousReadFileChannel must be called
before the first read I/O or otherwise if cancellation signals happens
immediately the onDispose callback may be missed.

The DefaultBufferFactory workaround however remains in place until an
expected additional fix arrives with Reactor Core 3.2.9.

See gh-22107
@rstoyanchev rstoyanchev referenced this issue Apr 5, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rstoyanchev rstoyanchev

Labels
in: web type: task
Projects
None yet
Milestone
  5.1.6
3 participants
@spring-issuemaster @rstoyanchev @poutsma
You can’t perform that action at this time.