Rewind POST content stream before using it again #608
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
If a POST request is redirected the redirection happens after the data has already been read from the request content object and thus the stream it was read from is positioned at its end. Since redirect reuses the request content object it can end up using the same Stream instance as in the previous request (the request content object might be caching it) and would end up getting an exception since the stream would be considered "read" or "closed. There are two possible fixes for this issue. One is to cache the POST content in a local buffer/stream and the other is to rewind the stream to beginning after reading from it in the original request. The first fix should be avoided in our scenario as the handler runs on memory constrained devices and should not use too much memory, and that may very well happen if the POST is sending a large file to the service. The second fix will fail if the stream isn't seekable, but hopefully this will happen few and far between, if at all. The advantage of this fix is that it is compatible with any way the HttpContent used in request handles its internal streams. For these reasons this commit implements the second fix. Additionally, this commit moves the code to a virtual method that can be overriden by anyone who needs to implement the POST data copying in a different way than implemented by us. Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=55477
|
build |
jonpryor
added a commit
to jonpryor/xamarin-android
that referenced
this pull request
Mar 27, 2020
Changes: dotnet/java-interop@1a086ff...56c92c7 * dotnet/java-interop@56c92c7: [build] Remove cecil submodule (dotnet#597) * dotnet/java-interop@3091274: [build] Provide a default $(Configuration) value (dotnet#612) * dotnet/java-interop@cf3e7c2: [generator] Don't process duplicate reference assemblies (dotnet#611) * dotnet/java-interop@f5fa462: [jnienv-gen] Convert to SDK-style (dotnet#608)
jonpryor
added a commit
that referenced
this pull request
Mar 27, 2020
Changes: dotnet/java-interop@1a086ff...56c92c7 * dotnet/java-interop@56c92c7: [build] Remove cecil submodule (#597) * dotnet/java-interop@3091274: [build] Provide a default $(Configuration) value (#612) * dotnet/java-interop@cf3e7c2: [generator] Don't process duplicate reference assemblies (#611) * dotnet/java-interop@f5fa462: [jnienv-gen] Convert to SDK-style (#608) Of particular note is [dotnet/java-interop@56c92c7][0], which replaces the `mono/cecil` submodule within Java.Interop with the [`Mono.Cecil` NuGet package][1] in an effort to simplify the Java.Interop build system. This simplifies the Java.Interop repo, and we *thought* that since xamarin-android *doesn't even use* Java.Interop's cecil submodule-built `Mono.Cecil.dll` -- instead the `Mono.Cecil.dll` from the "mono archive" is "renamed" to `Xamarin.Android.Cecil.dll` during `make prepare` (0c9f83b) -- surely this would be a simple change. The removal of the cecil submodule also required changing `ThirdPartyNotice.txt` generation so that the LICENSE for Cecil was obtained from the mono archive instead of from Java.Interop. Unfortunately, the integration was a tad more complicated than anticipated. With the ongoing adoption of MSBuild multi-targeting and builds against the `netcoreapp3.1` target framework -- commit e2854ee and numerous commits in Java.Interop -- we encountered a problem with MSBuild semantics: If two `$(TargetFramework)` builds share the same output directory, the `IncrementalClean` target will *remove files created by previous builds*, e.g. when e.g. `Java.Interop/tools/generator.csproj` builds the `netcoreapp3.1` framework, it will *delete* the `generator.exe` built by the `net472` framework, which results in subsequent build breaks. The only path to sanity is to *ensure* that different `$(TargetFramework)` builds have *completely separate* `$(OutputPath)` values. The "normal" approach to doing this is for `$(OutputPath)` to end with `$(TargetFramework)`, which is the case when `$(AppendTargetFrameworkToOutputPath)`=True (the default). Unfortunately in xamarin-android we don't want `$(OutputPath)` to end with `$(TargetFramework)`; we want the build tree structure to mirror the installation directory structure, which -- at present -- doesn't mention `$(TargetFramework)` at all. The solution here is to use "non-overlapping" directories. For example, in e2854ee there are "two" `$(OutputPath)` values: * `MonoAndroid10.0`: `bin/Debug/lib/xamarin.android/xbuild-frameworks/MonoAndroid/v10.0/Mono.Android.dll` * `netcoreapp3.1`: `bin/Debug/lib/xamarin.android/xbuild-frameworks/Xamarin.Android.App/netcoreapp3.1/Mono.Android.dll` The same "non-overlapping directories" strategy needs to be extended to *all* multi-targeted projects from Java.Interop, *including* dependencies. Dependencies such as `Xamarin.Android.Cecil.dll`. Define a new `$(UtilityOutputFullPathCoreApps)` MSBuild property so that Java.Interop "utility" project builds, when building for the `netcoreapp3.1` framework, use a *different* `Xamarin.Android.Cecil.dll` than is used with the `net472`-related builds. Update `xaprepare` to *create* this new `netcoreapp3.1`-correlated `Xamarin.Android.Cecil.dll`. It's the same file, just in a different directory, to prevent "accidental" deletes by `IncrementalClean`. Even with all that, MSBuild still had other ideas. In particular, MSBuild wasn't particularly happy about our attempt to use the `$(UtilityOutputFullPath)` property to "switch" between using a `@(PackageReference)` to the Mono.Cecil NuGet package vs. using a `@(Reference)` to the `Xamarin.Android.Cecil.dll` assembly, because MSBuild *caches* this information somewhere within `obj` directories. To get MSBuild to re-evaluate it's assembly reference choices, we must instead replace `msbuild` with `msbuild /restore`. Which still isn't enough, because some of our MSBuild invocations are via the `<MSBuild/>` task, within `msbuild`. To get *that* working, we need to explicitly invoke the `Restore` target through a *separate* `<MSBuild/>` task invocation. You ***CANNOT*** use `<MSBuild Targets="Restore;Build" />`, as "obvious" as that may be, because it [doesn't work reliably][2]. ([Yet.][3]) [0]: dotnet/java-interop@56c92c7 [1]: https://www.nuget.org/packages/Mono.Cecil/0.11.2 [2]: dotnet/msbuild#3000 (comment) [3]: dotnet/msbuild#2811
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If a POST request is redirected the redirection happens after the data has
already been read from the request content object and thus the stream it was
read from is positioned at its end. Since redirect reuses the request content
object it can end up using the same Stream instance as in the previous
request (the request content object might be caching it) and would end up
getting an exception since the stream would be considered "read" or "closed.
There are two possible fixes for this issue. One is to cache the POST content in
a local buffer/stream and the other is to rewind the stream to beginning after
reading from it in the original request.
The first fix should be avoided in our scenario as the handler runs on memory
constrained devices and should not use too much memory, and that may very well
happen if the POST is sending a large file to the service.
The second fix will fail if the stream isn't seekable, but hopefully this will
happen few and far between, if at all. The advantage of this fix is that it is
compatible with any way the HttpContent used in request handles its internal
streams. For these reasons this commit implements the second fix.
Additionally, this commit moves the code to a virtual method that can be
overriden by anyone who needs to implement the POST data copying in a different
way than implemented by us.
Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=55477