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.

Sign up for GitHub

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

Add support for fixed ContentLength #32

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+103 −2
Open

Add support for fixed ContentLength #32

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0a00d7b
Add support for fixed ContentLength
gbonnefille Feb 6, 2020
9c518a4
Remove old FIXME
gbonnefille Feb 10, 2020
a23d56a
Control for chunked transfer encoding from client
gbonnefille Feb 12, 2020
a5754cf
Add a finalization reply
gbonnefille Feb 12, 2020
9940d3c
Fix naming convention
gbonnefille May 6, 2020
9daad1e
Fix code convention
gbonnefille May 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions src/main/java/io/tus/java/client/TusClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ public class TusClient {
private URL uploadCreationURL;
private boolean resumingEnabled;
private boolean removeFingerprintOnSuccessEnabled;
private boolean chunkedTransferEncoding = true;
private TusURLStore urlStore;
private Map<String, String> headers;
private int connectTimeout = 5000;
Expand Down Expand Up @@ -115,6 +116,39 @@ public boolean removeFingerprintOnSuccessEnabled() {
return removeFingerprintOnSuccessEnabled;
}

/**
* Enable chunked transfer encoding.
Copy link
Member

Choose a reason for hiding this comment

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

Could you expand the comment a bit to explain what the advantages/disadvantages/impacts of enabling/disabling chunked transfer encoding has?

*
* @see https://en.wikipedia.org/wiki/Chunked_transfer_encoding
* @see #disableChunkedTransferEncoding()
*/
public void enableChunkedTransferEncoding() {
chunkedTransferEncoding = true;
}

/**
* Disable chunked transfer encoding.
*
* @see https://en.wikipedia.org/wiki/Chunked_transfer_encoding
* @see #enableChunkedTransferEncoding()
*/
public void disableChunkedTransferEncoding() {
chunkedTransferEncoding = false;
}

/**
* Get the current status of chunked transfer encoding.
*
* @see https://en.wikipedia.org/wiki/Chunked_transfer_encoding
* @see #enableChunkedTransferEncoding()
* @see #disableChunkedTransferEncoding()
*
* @return True if chunked transfer encoding has been enabled.
*/
public boolean chunkedTransferEncoding() {
gbonnefille marked this conversation as resolved.
Show resolved Hide resolved
return chunkedTransferEncoding;
}


/**
* Set headers which will be added to every HTTP requestes made by this TusClient instance.
Expand Down
13 changes: 11 additions & 2 deletions src/main/java/io/tus/java/client/TusUploader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ private void openConnection() throws IOException, ProtocolException {
}

connection.setDoOutput(true);
connection.setChunkedStreamingMode(0);
if (client.chunkedTransferEncoding())
gbonnefille marked this conversation as resolved.
Show resolved Hide resolved
connection.setChunkedStreamingMode(0);
try {
output = connection.getOutputStream();
} catch(java.net.ProtocolException pe) {
Expand Down Expand Up @@ -176,7 +177,15 @@ public int getRequestPayloadSize() {
public int uploadChunk() throws IOException, ProtocolException {
openConnection();

int bytesToRead = Math.min(getChunkSize(), bytesRemainingForRequest);
final int bytesToRead;
if (client.chunkedTransferEncoding())
bytesToRead = Math.min(getChunkSize(), bytesRemainingForRequest);
else {
// No chunked tranfer encoding, so we upload a single chunk.
bytesToRead = getChunkSize();
Copy link
Member

Choose a reason for hiding this comment

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

I think there is still some confusion about the chunk size vs payload size. Traditionally the payload size determines how big the body of a HTTP can get. The chunk size on the other hand says, how much data is read from the input stream and written to the TCP connection in one iteration. Usually, you write multiple chunks per HTTP request, so the payload size is bigger than the chunk size.

However, if I understand this correctly, the chunk size now determines how big a HTTP request gets and the payload size is not used anymore. This would be contrary to how tus-java-client behaves when chunked transfer is enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did the opposite reasoning:
chunk size exists because something in the chain does not support more than that. So, when chunk transfer is not supported, it seems better to use TUS protocol in place of chunk transfer and then, send many complete HTTP+TUS messages based on chunk size.

Furthermore, as a dumb user, I only wish things work without having to fine tune chunk size or payload size, depending on the server's capabilities. I like automagical approach. :-)

But I'm completly new to all of this. So I don't know what sort of logic is expected.

Copy link
Member

Choose a reason for hiding this comment

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

chunk size exists because something in the chain does not support more than that.

That's not entirely true. The reason for chunk size is that the user can control how much data is read from the input source in one go. This is important to control how big the in-memory buffer of tus-java-client is, in order to avoid excess memory usage.

You are referring to the payload size when you say "something [e.g. a proxy] in the chain does not support more than that". Does that make sense?

Furthermore, as a dumb user, I only wish things work without having to fine tune chunk size or payload size, depending on the server's capabilities. I like automagical approach. :-)

Yes, I would like that as well and it will likely come in the next major release but not right now :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is important to control how big the in-memory buffer of tus-java-client is, in order to avoid excess memory usage.

OK, so you state that client (memory) is "something in the chain does not support more than that", first part of the chain.

// The connection should be closed after single write.
bytesRemainingForRequest = 0;
}

int bytesRead = input.read(buffer, bytesToRead);
if(bytesRead == -1) {
Expand Down
57 changes: 57 additions & 0 deletions src/test/java/io/tus/java/client/TestTusUploader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,63 @@ public void testTusUploader() throws IOException, ProtocolException {
uploader.finish();
}

@Test
public void testTusUploaderFixedLength() throws IOException, ProtocolException {
byte[] content = "hello world".getBytes();

mockServer.when(new HttpRequest()
.withPath("/files/fixed")
.withHeader("Tus-Resumable", TusClient.TUS_VERSION)
.withHeader("Upload-Offset", "5")
.withHeader("Content-Type", "application/offset+octet-stream")
.withHeader(isOpenJDK6 ? "": "Expect: 100-continue")
.withBody(Arrays.copyOfRange(content, 5, 10)))
.respond(new HttpResponse()
.withStatusCode(204)
.withHeader("Tus-Resumable", TusClient.TUS_VERSION)
.withHeader("Upload-Offset", "10"));
mockServer.when(new HttpRequest()
.withPath("/files/fixed")
.withHeader("Tus-Resumable", TusClient.TUS_VERSION)
.withHeader("Upload-Offset", "10")
.withHeader("Content-Type", "application/offset+octet-stream")
.withHeader(isOpenJDK6 ? "": "Expect: 100-continue")
.withBody(Arrays.copyOfRange(content, 10, 11)))
.respond(new HttpResponse()
.withStatusCode(204)
.withHeader("Tus-Resumable", TusClient.TUS_VERSION)
.withHeader("Upload-Offset", "11"));
mockServer.when(new HttpRequest()
.withPath("/files/fixed")
.withHeader("Tus-Resumable", TusClient.TUS_VERSION)
.withHeader("Upload-Offset", "11")
.withHeader("Content-Type", "application/offset+octet-stream")
.withHeader(isOpenJDK6 ? "": "Expect: 100-continue"))
.respond(new HttpResponse()
.withStatusCode(204)
.withHeader("Tus-Resumable", TusClient.TUS_VERSION)
.withHeader("Upload-Offset", "11"));

TusClient client = new TusClient();
client.disableChunkedTransferEncoding();
URL uploadUrl = new URL(mockServerURL + "/fixed");
TusInputStream input = new TusInputStream(new ByteArrayInputStream(content));
long offset = 5;

TusUpload upload = new TusUpload();

TusUploader uploader = new TusUploader(client, upload, uploadUrl, input, offset);

uploader.setChunkSize(5);
assertEquals(uploader.getChunkSize(), 5);

assertEquals(5, uploader.uploadChunk());
assertEquals(1, uploader.uploadChunk());
assertEquals(-1, uploader.uploadChunk());
assertEquals(11, uploader.getOffset());
uploader.finish();
}

@Test
public void testTusUploaderClientUploadFinishedCalled() throws IOException, ProtocolException {

Expand Down