Skip to content

Conversation

@Ndiritu
Copy link
Contributor

@Ndiritu Ndiritu commented Dec 9, 2024

We added the isOneShot() check to ensure that rewindable request body streams are reset after writing them e.g. after request body is written to a logging interceptor. Previously, a logging interceptor would drain the request body stream causing the network call to have an empty body.

However, we didn't handle the failure scenarios of reset() according to the docs

This PR makes a best effort measure to prevent draining the request body by an interceptor before the final network request.

If a positive content length is available, we mark() the start of the stream with the entire stream's content length as the read limit.

If during the sink.writeAll() a new mark() is set causing the reset() to fail, we log the exception but don't fail the entire request.

We do not fail the entire request in case the first call to reset() happens during the network call.

closes microsoftgraph/msgraph-sdk-java#2245

Existing unit tests for the various stream scenarios still pass

@Ndiritu Ndiritu self-assigned this Dec 9, 2024
@Ndiritu Ndiritu force-pushed the fix/request-body-stream-reset branch from ab60655 to 2bf7173 Compare December 9, 2024 20:42
@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 9, 2024

@ihudedi
Copy link

ihudedi commented Dec 11, 2024

Hi @Ndiritu
I saw another call to reset in this file and exception is thrown

Thanks,
Itay

@ihudedi
Copy link

ihudedi commented Dec 15, 2024

Hi @Ndiritu
Did you see my last message.In retry method there is also reset that throw an exception.(retryCAEResponseIfRequired)
Thanks,
Itay

@Ndiritu Ndiritu marked this pull request as ready for review December 17, 2024 15:51
@Ndiritu Ndiritu requested a review from a team as a code owner December 17, 2024 15:51
@Ndiritu
Copy link
Contributor Author

Ndiritu commented Dec 17, 2024

Hi @Ndiritu Did you see my last message.In retry method there is also reset that throw an exception.(retryCAEResponseIfRequired) Thanks, Itay

@ihudedi the reset() called at this point is necessary since before retrying a request after getting a claims challenge we need to attempt to reset the body.

In this case, the error handling works as expected by bubbling up the exception to a developer's application.

@ihudedi
Copy link

ihudedi commented Dec 17, 2024

Hi @Ndiritu
Thanks for the response.
Itay

@Ndiritu Ndiritu merged commit 87dc6b8 into main Dec 19, 2024
10 checks passed
@Ndiritu Ndiritu deleted the fix/request-body-stream-reset branch December 19, 2024 13:24
@Ndiritu Ndiritu mentioned this pull request Dec 19, 2024
@Ndiritu Ndiritu mentioned this pull request Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Upload file to Sharepoint failed with 'Stream mark expired' when the inputstream is markSuppoted

4 participants