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

Fix handling of artifact range requests #1445

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,18 @@

import java.io.BufferedInputStream;
import java.io.File;
import java.io.RandomAccessFile;
import java.nio.channels.Channels;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
import java.util.Objects;

import javax.validation.constraints.NotNull;

import com.google.common.io.ByteStreams;

import org.eclipse.hawkbit.artifact.repository.model.AbstractDbArtifact;
import org.eclipse.hawkbit.artifact.repository.model.DbArtifactHash;

Expand Down Expand Up @@ -47,4 +52,19 @@ public InputStream getFileInputStream() {
throw new ArtifactFileNotFoundException(e);
}
}

@Override
// suppress warning, this InputStream needs to be closed by the caller, this
// cannot be closed in this method
@SuppressWarnings("squid:S2095")
public InputStream getFileInputStream(long start, long end) {
try {
var f = new RandomAccessFile(file, "r");
var ch = f.getChannel();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather not add an additional dependency on com.google.common
wouldn't

f.seek(start);
return f;

do the same?

ch.position(start);
return ByteStreams.limit(Channels.newInputStream(ch), end - start + 1);
} catch (final IOException e) {
throw new ArtifactFileNotFoundException(e);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,13 @@ public interface DbArtifact {
* @return {@link InputStream} to read from artifact.
*/
InputStream getFileInputStream();

/**
* Creates an {@link InputStream} on a single range of this artifact.
* The caller has to take care of closing the stream.
* Repeatable calls open a new {@link InputStream}.
*
* @return {@link InputStream} to read from artifact.
*/
InputStream getFileInputStream(long start, long end);
Copy link
Contributor

Choose a reason for hiding this comment

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

here an default implementation could be provided - one that skips the reading of input stream start.

}
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,9 @@ public String getContentType() {
public InputStream getFileInputStream() {
return decryptionFunction.apply(encryptedDbArtifact.getFileInputStream());
}

@Override
public InputStream getFileInputStream(long start, long end) {
Copy link
Contributor

Choose a reason for hiding this comment

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

start and end are pointers in the plain (decrypted input stream).
here in encryptedDbArtifact.getFileInputStream(start, end) they are applied to the encrypted stream where the offsets could differ.
Also, decryption function could be stateful and depend on the previous content.
I'd propose to implement implement default range function and to use it for that DB

return decryptionFunction.apply(encryptedDbArtifact.getFileInputStream(start, end));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -285,8 +285,8 @@ private static ResponseEntity<InputStream> handleMultipartRangeRequest(final DbA
final ServletOutputStream to = response.getOutputStream();

for (final ByteRange r : ranges) {
try (final InputStream from = artifact.getFileInputStream()) {

try (final InputStream from = artifact.getFileInputStream(r.getStart(), r.getEnd())) {
// Add multipart boundary and header fields for every range.
to.println();
to.println("--" + ByteRange.MULTIPART_BOUNDARY);
Expand Down Expand Up @@ -316,7 +316,7 @@ private static ResponseEntity<InputStream> handleStandardRangeRequest(final DbAr
response.setContentLengthLong(r.getLength());
response.setStatus(HttpServletResponse.SC_PARTIAL_CONTENT);

try (final InputStream from = artifact.getFileInputStream()) {
try (final InputStream from = artifact.getFileInputStream(r.getStart(), r.getEnd())) {
final ServletOutputStream to = response.getOutputStream();
copyStreams(from, to, progressListener, r.getStart(), r.getLength(), filename);
} catch (final IOException e) {
Expand All @@ -340,8 +340,6 @@ private static long copyStreams(final InputStream from, final OutputStream to,
long total = 0;
int progressPercent = 1;

ByteStreams.skipFully(from, start);
Copy link
Contributor

Choose a reason for hiding this comment

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

since this method, now reads from 0 to length (exclusively) "final long start" is little bit misleading.
I'd propose to remove start and file name parameters and to log start/end in callers
Or at least rename start / length to rangeStart / rangeLength so the start param to be not correlated to input stream start offset


long toRead = length;
boolean toContinue = true;
long shippedSinceLastEvent = 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,11 @@ public String getContentType() {
public InputStream getFileInputStream() {
return new ByteArrayInputStream(CONTENT_BYTES);
}

@Override
public InputStream getFileInputStream(long start, long end) {
return new ByteArrayInputStream(CONTENT_BYTES, (int) start, (int) (end - start + 1));
}
};

@Test
Expand Down