fix: Don't close upstream on HttpFile::Flush #1201
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
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Closing the upstream on flush will effectively terminate the ongoing curl connection. This means that we would need re-establish the connection in order to resume writing, this is not what we want. In the spirit of the documentation of File::Flush /// Flush the file so that recently written data will survive an /// application crash (but not necessarily an OS crash). For /// instance, in LocalFile the data is flushed into the OS but not /// necessarily to disk. We will instead wait for the curl thread to finish consuming what ever might be in the upload cache, but leave the connection open for subsequent writes.
petzeb
force-pushed
the
dont_close_http_upstream_on_flush
branch
from
bd957c3
to
1b5429e
Compare
petzeb
changed the title
|
This is an attempt to fix #1196 |
|
Should I tag this PR as closing issue #1196? Could you add a unit test for this? |
Yes, if this approach seems reasonable. I am quite new to the shaka-packager code base.
I'll see what I can do 👍 |
|
@joeyparrish Sorry for the delay, I was just about to start looking at adding a unit test but I noticed now that all the unit tests for http_file seem to be disabled. Any idea why this is? |
|
I don't know the history for those tests being disabled, but in the I will merge this as is, but follow up with a new test in the |
joeyparrish
approved these changes
Jul 5, 2023
|
This is causing issues with upload tests. Essentially, they hang forever because curl thinks there will be more chunks to upload. I see how your change is more in line with the original intent of Flush(), and I have yet to find evidence of Flush() being used in the normal pipeline to write data to the server during an upload. It should perhaps be Close() that signals the last chunk has been sent, but there will need to be more changes to make that work, since right now Close() terminates the HTTP connection, possibly without reading the response. |
joeyparrish
added a commit
to joeyparrish/shaka-packager
that referenced
this pull request
Jul 13, 2023
All HTTP-based tests now use an embedded test server instead of httpbin.org, which makes them much faster and more reliable. These more reliable tests also exposed some issues that began recently with PR shaka-project#1201. HttpFile's Flush() semantics were different than those documented for files in general. Flush() used to close the file for uploading, so that no further writes were allowed, but the documentation stated that it would only flush data to its destination. PR shaka-project#1201 brought HttpFile's Flush() in line with the docs, but gave us no way to terminate a chunked upload. This adds a new method to File called CloseForWriting(), which terminates a chunked upload for HttpFile. The only other implementation that does anything is UdpFile, which uses the socket library function shutdown() to terminate writes while allowing reads. This also tweaks HttpFile::CloseWithStatus() so that it will not generate an error if the file is closed before the HTTP response is written to the download cache. This modifies the test HttpFileTest.MultipleWrites so that the file is Flushed after each chunk. This adds test coverage for the changes introduced in PR shaka-project#1201. Fixes shaka-project#1224 (missing test coverage for HttpFile::Flush)
joeyparrish
added a commit
to joeyparrish/shaka-packager
that referenced
this pull request
Jul 13, 2023
All HTTP-based tests now use an embedded test server instead of httpbin.org, which makes them much faster and more reliable. These more reliable tests also exposed some issues that began recently with PR shaka-project#1201. HttpFile's Flush() semantics were different than those documented for files in general. Flush() used to close the file for uploading, so that no further writes were allowed, but the documentation stated that it would only flush data to its destination. PR shaka-project#1201 brought HttpFile's Flush() in line with the docs, but gave us no way to terminate a chunked upload. This adds a new method to File called CloseForWriting(), which terminates a chunked upload for HttpFile. The only other implementation that does anything is UdpFile, which uses the socket library function shutdown() to terminate writes while allowing reads. This also tweaks HttpFile::CloseWithStatus() so that it will not generate an error if the file is closed before the HTTP response is written to the download cache. This modifies the test HttpFileTest.MultipleWrites so that the file is Flushed after each chunk. This adds test coverage for the changes introduced in PR shaka-project#1201. Fixes shaka-project#1224 (missing test coverage for HttpFile::Flush)
joeyparrish
added a commit
to joeyparrish/shaka-packager
that referenced
this pull request
Jul 13, 2023
All HTTP-based tests now use an embedded test server instead of httpbin.org, which makes them much faster and more reliable. These more reliable tests also exposed some issues that began recently with PR shaka-project#1201. HttpFile's Flush() semantics were different than those documented for files in general. Flush() used to close the file for uploading, so that no further writes were allowed, but the documentation stated that it would only flush data to its destination. PR shaka-project#1201 brought HttpFile's Flush() in line with the docs, but gave us no way to terminate a chunked upload. This adds a new method to File called CloseForWriting(), which terminates a chunked upload for HttpFile. The only other implementation that does anything is UdpFile, which uses the socket library function shutdown() to terminate writes while allowing reads. This also tweaks HttpFile::CloseWithStatus() so that it will not generate an error if the file is closed before the HTTP response is written to the download cache. This modifies the test HttpFileTest.MultipleWrites so that the file is Flushed after each chunk. This adds test coverage for the changes introduced in PR shaka-project#1201. Fixes shaka-project#1224 (missing test coverage for HttpFile::Flush)
joeyparrish
added a commit
that referenced
this pull request
Jul 14, 2023
All HTTP-based tests now use an embedded test server instead of httpbin.org, which makes them much faster and more reliable. These more reliable tests also exposed some issues that began recently with PR #1201. HttpFile's Flush() semantics were different than those documented for files in general. Flush() used to close the file for uploading, so that no further writes were allowed, but the documentation stated that it would only flush data to its destination. PR #1201 brought HttpFile's Flush() in line with the docs, but gave us no way to terminate a chunked upload. This adds a new method to File called CloseForWriting(), which terminates a chunked upload for HttpFile. The only other implementation that does anything is UdpFile, which uses the socket library function shutdown() to terminate writes while allowing reads. This also tweaks HttpFile::CloseWithStatus() so that it will not generate an error if the file is closed before the HTTP response is written to the download cache. This modifies the test HttpFileTest.MultipleWrites so that the file is Flushed after each chunk. This adds test coverage for the changes introduced in PR #1201. Fixes #1224 (missing test coverage for HttpFile::Flush)
github-actions
bot
added
the
status: archived
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.
Closing the upstream on flush will effectively terminate the ongoing curl connection. This means that we would need re-establish the connection in order to resume writing, this is not what we want. In the spirit of the documentation of File::Flush
We will instead wait for the curl thread to finish consuming what ever might be in the upload cache, but leave the connection open for subsequent writes.
Fixes #1196