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

#921 Allow reading body multiple times #923

Merged
merged 24 commits into from
Jun 24, 2024

Conversation

andriy-dmytruk
Copy link
Contributor

The proposed approach to do the same as in micronaut-projects/micronaut-gcp#1106 did not exactly work. This is because the InputEvent does not allow accessing the InputStream directly. Instead it has a method consumeBody(Function<InputStream>) that should be called with implementation similar to this:

<T> T consumeBody(Function<InputStream, T> function) {
      /* verify that not null and not opened twice ... */
      try {
          return function.call(this.body);
      } finally {
          this.body.close();
      }
      /* ... */
}

Because it closes the stream we cannot read the stream gradually and split it.

The solution is to read the stream as a whole into a byte array and then provide it to all the consumers. This will probably result in the same memory usage as in https://github.com/micronaut-projects/micronaut-gcp/pull/1106/files (since there we split each time and end up with one unused ByteBody caching the whole request). However, it will require reading the whole body before it is consumed (e.g. JSON parser cannot begin parsing the stream before whole body is read into memory).

@andriy-dmytruk andriy-dmytruk requested a review from yawkat June 13, 2024 19:50
@andriy-dmytruk
Copy link
Contributor Author

It seems like with this fix: micronaut-projects/micronaut-servlet#740 we can have an implementation using InputStreamByteBody. Not sure if this is better now.

@andriy-dmytruk andriy-dmytruk force-pushed the andriy/function-http-tck-multiread-body branch from 8701a10 to ab4601e Compare June 14, 2024 18:31
Base automatically changed from andriy/function-http-tck to 4.1.x June 18, 2024 12:23
@yawkat
Copy link
Member

yawkat commented Jun 18, 2024

@andriy-dmytruk PTAL. My byteBody approach is detailed in this comment: fd99aaa#diff-223b98dc29906194b0c41eeb83d19e55664789c8f55eca639a20b3e9c6ccdf51R114-R132


To work around this limitation, we do a single big consumeBody around the entire request
processing. This hopefully encompasses most users of the body stream. Any read operations
inside this block are simply forwarded upstream.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, this is great!

@andriy-dmytruk andriy-dmytruk force-pushed the andriy/function-http-tck-multiread-body branch from fd99aaa to fec7e83 Compare June 18, 2024 16:05
@andriy-dmytruk
Copy link
Contributor Author

Rebased on latest 4.1.x branch

Copy link

❌ Oracle Cloud CI 17 latest failed: https://ge.micronaut.io/s/cqqo3c6ixfpyi

@andriy-dmytruk andriy-dmytruk changed the title #921 Attempt to allow reading body multiple times #921 Allow reading body multiple times Jun 18, 2024
@yawkat
Copy link
Member

yawkat commented Jun 19, 2024

@graemerocher please review and merge

yawkat added 2 commits June 19, 2024 08:18
…ad-body' into andriy/function-http-tck-multiread-body

# Conflicts:
#	gradle/libs.versions.toml
#	test-suite-http-server-tck-oraclecloud-function-http/build.gradle.kts
Copy link

❌ Oracle Cloud CI 17 latest failed: https://ge.micronaut.io/s/52tpddmkuystm

@graemerocher
Copy link
Contributor

There are test failures

@yawkat
Copy link
Member

yawkat commented Jun 19, 2024

seems there's still an issue with the FnBodyBinder, but that can be fixed in another pr. i've disabled the tck test again

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
68.8% Coverage on New Code (required ≥ 70%)
1 New Blocker Issues (required ≤ 0)
1 New Critical Issues (required ≤ 0)
1 New Bugs (required ≤ 0)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@andriy-dmytruk
Copy link
Contributor Author

@graemerocher Can we merge?

@graemerocher graemerocher merged commit e023213 into 4.1.x Jun 24, 2024
20 of 21 checks passed
@graemerocher graemerocher deleted the andriy/function-http-tck-multiread-body branch June 24, 2024 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants