You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-dev@jackrabbit.apache.org by "rishabhdaim (via GitHub)" <gi...@apache.org> on 2026/01/19 09:06:01 UTC
[PR] OAK-12062 : fixed NPE while uploading metadata for AWS S3 [jackrabbit-oak]
rishabhdaim opened a new pull request, #2688:
URL: https://github.com/apache/jackrabbit-oak/pull/2688
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-12062 : fixed NPE while uploading metadata for AWS S3 [jackrabbit-oak]
Posted by "rishabhdaim (via GitHub)" <gi...@apache.org>.
rishabhdaim merged PR #2688:
URL: https://github.com/apache/jackrabbit-oak/pull/2688
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-12062 : fixed NPE while uploading metadata for AWS S3 [jackrabbit-oak]
Posted by "thomasmueller (via GitHub)" <gi...@apache.org>.
thomasmueller commented on code in PR #2688:
URL: https://github.com/apache/jackrabbit-oak/pull/2688#discussion_r2705131605
##########
oak-blob-cloud/src/main/java/org/apache/jackrabbit/oak/blob/cloud/s3/S3Backend.java:
##########
@@ -1334,42 +1334,38 @@ private static String getIdentifierName(String key) {
@NotNull
private AsyncRequestBody getRequestBody(final InputStream input, final ExecutorService executor,
final PutObjectRequest.Builder builder) throws IOException {
+ // for both AWS/GCP we need to know the length in advance, else it won't work.
final AsyncRequestBody body;
- if (Objects.equals(RemoteStorageMode.S3, properties.get(S3Constants.MODE))) {
- body = AsyncRequestBody.fromInputStream(input, null, executor);
+ final long length;
+ if (input instanceof FileInputStream) {
+ final FileInputStream fis = (FileInputStream) input;
+ // if the file is modified after opening, the size may not reflect the latest changes
+ length = fis.getChannel().size();
+ body = AsyncRequestBody.fromInputStream(input, length, executor);
+ } else if (input instanceof ByteArrayInputStream) {
+ length = input.available();
+ body = AsyncRequestBody.fromInputStream(input, length, executor);
+ } else if (input.markSupported()) {
+ // in case the inputStream supports mark & reset
+ input.mark(Integer.MAX_VALUE);
+ length = IOUtils.consume(input);
+ input.reset();
+ body = AsyncRequestBody.fromInputStream(input, length, executor);
} else {
- // for GCP we need to know the length in advance, else it won't work.
- final long length;
- if (input instanceof FileInputStream) {
- final FileInputStream fis = (FileInputStream) input;
- // if the file is modified after opening, the size may not reflect the latest changes
- length = fis.getChannel().size();
- body = AsyncRequestBody.fromInputStream(input, length, executor);
- } else if (input instanceof ByteArrayInputStream) {
- length = input.available();
- body = AsyncRequestBody.fromInputStream(input, length, executor);
- } else if (input.markSupported()) {
- // in case the inputStream supports mark & reset
- input.mark(Integer.MAX_VALUE);
- length = IOUtils.consume(input);
- input.reset();
- body = AsyncRequestBody.fromInputStream(input, length, executor);
- } else {
- // we have to read all the stream to get the actual length
- // last else block: store to temp file and re-read
- final File tempFile = File.createTempFile("inputstream-", ".tmp");
- tempFile.deleteOnExit(); // Clean up after JVM exits
+ // we have to read all the stream to get the actual length
+ // last else block: store to temp file and re-read
+ final File tempFile = File.createTempFile("inputstream-", ".tmp");
Review Comment:
How is it addressed? I do not see where "delete()" is called. It is still only calling "deleteOnExit()", which will not delete immediately.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-12062 : fixed NPE while uploading metadata for AWS S3 [jackrabbit-oak]
Posted by "reschke (via GitHub)" <gi...@apache.org>.
reschke commented on PR #2688:
URL: https://github.com/apache/jackrabbit-oak/pull/2688#issuecomment-3767484472
I'm a bit concerned that we have an input stream leak; `deleteOnExit()` will not be sufficient unless the VM really terminates.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-12062 : fixed NPE while uploading metadata for AWS S3 [jackrabbit-oak]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #2688:
URL: https://github.com/apache/jackrabbit-oak/pull/2688#issuecomment-3768673980
## [](https://sonarcloud.io/dashboard?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=2688) **Quality Gate failed**
Failed conditions
 [1 Security Hotspot](https://sonarcloud.io/project/security_hotspots?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=2688&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=2688&metric=new_coverage&view=list) (required ≥ 80%)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=org.apache.jackrabbit%3Ajackrabbit-oak&pullRequest=2688)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-12062 : fixed NPE while uploading metadata for AWS S3 [jackrabbit-oak]
Posted by "rishabhdaim (via GitHub)" <gi...@apache.org>.
rishabhdaim commented on code in PR #2688:
URL: https://github.com/apache/jackrabbit-oak/pull/2688#discussion_r2704662295
##########
oak-blob-cloud/src/main/java/org/apache/jackrabbit/oak/blob/cloud/s3/S3Backend.java:
##########
@@ -1334,42 +1334,38 @@ private static String getIdentifierName(String key) {
@NotNull
private AsyncRequestBody getRequestBody(final InputStream input, final ExecutorService executor,
final PutObjectRequest.Builder builder) throws IOException {
+ // for both AWS/GCP we need to know the length in advance, else it won't work.
final AsyncRequestBody body;
- if (Objects.equals(RemoteStorageMode.S3, properties.get(S3Constants.MODE))) {
- body = AsyncRequestBody.fromInputStream(input, null, executor);
+ final long length;
+ if (input instanceof FileInputStream) {
+ final FileInputStream fis = (FileInputStream) input;
+ // if the file is modified after opening, the size may not reflect the latest changes
+ length = fis.getChannel().size();
+ body = AsyncRequestBody.fromInputStream(input, length, executor);
+ } else if (input instanceof ByteArrayInputStream) {
+ length = input.available();
+ body = AsyncRequestBody.fromInputStream(input, length, executor);
+ } else if (input.markSupported()) {
+ // in case the inputStream supports mark & reset
+ input.mark(Integer.MAX_VALUE);
+ length = IOUtils.consume(input);
+ input.reset();
+ body = AsyncRequestBody.fromInputStream(input, length, executor);
} else {
- // for GCP we need to know the length in advance, else it won't work.
- final long length;
- if (input instanceof FileInputStream) {
- final FileInputStream fis = (FileInputStream) input;
- // if the file is modified after opening, the size may not reflect the latest changes
- length = fis.getChannel().size();
- body = AsyncRequestBody.fromInputStream(input, length, executor);
- } else if (input instanceof ByteArrayInputStream) {
- length = input.available();
- body = AsyncRequestBody.fromInputStream(input, length, executor);
- } else if (input.markSupported()) {
- // in case the inputStream supports mark & reset
- input.mark(Integer.MAX_VALUE);
- length = IOUtils.consume(input);
- input.reset();
- body = AsyncRequestBody.fromInputStream(input, length, executor);
- } else {
- // we have to read all the stream to get the actual length
- // last else block: store to temp file and re-read
- final File tempFile = File.createTempFile("inputstream-", ".tmp");
- tempFile.deleteOnExit(); // Clean up after JVM exits
+ // we have to read all the stream to get the actual length
+ // last else block: store to temp file and re-read
+ final File tempFile = File.createTempFile("inputstream-", ".tmp");
+ tempFile.deleteOnExit(); // Clean up after JVM exits
Review Comment:
addressed in https://github.com/apache/jackrabbit-oak/pull/2688/commits/8f64b8a8e32ceb92024fde9998bdfba63220e5f3
##########
oak-blob-cloud/src/main/java/org/apache/jackrabbit/oak/blob/cloud/s3/S3Backend.java:
##########
@@ -1334,42 +1334,38 @@ private static String getIdentifierName(String key) {
@NotNull
private AsyncRequestBody getRequestBody(final InputStream input, final ExecutorService executor,
final PutObjectRequest.Builder builder) throws IOException {
+ // for both AWS/GCP we need to know the length in advance, else it won't work.
final AsyncRequestBody body;
- if (Objects.equals(RemoteStorageMode.S3, properties.get(S3Constants.MODE))) {
- body = AsyncRequestBody.fromInputStream(input, null, executor);
+ final long length;
+ if (input instanceof FileInputStream) {
+ final FileInputStream fis = (FileInputStream) input;
+ // if the file is modified after opening, the size may not reflect the latest changes
+ length = fis.getChannel().size();
+ body = AsyncRequestBody.fromInputStream(input, length, executor);
+ } else if (input instanceof ByteArrayInputStream) {
+ length = input.available();
+ body = AsyncRequestBody.fromInputStream(input, length, executor);
+ } else if (input.markSupported()) {
+ // in case the inputStream supports mark & reset
+ input.mark(Integer.MAX_VALUE);
+ length = IOUtils.consume(input);
+ input.reset();
+ body = AsyncRequestBody.fromInputStream(input, length, executor);
} else {
- // for GCP we need to know the length in advance, else it won't work.
- final long length;
- if (input instanceof FileInputStream) {
- final FileInputStream fis = (FileInputStream) input;
- // if the file is modified after opening, the size may not reflect the latest changes
- length = fis.getChannel().size();
- body = AsyncRequestBody.fromInputStream(input, length, executor);
- } else if (input instanceof ByteArrayInputStream) {
- length = input.available();
- body = AsyncRequestBody.fromInputStream(input, length, executor);
- } else if (input.markSupported()) {
- // in case the inputStream supports mark & reset
- input.mark(Integer.MAX_VALUE);
- length = IOUtils.consume(input);
- input.reset();
- body = AsyncRequestBody.fromInputStream(input, length, executor);
- } else {
- // we have to read all the stream to get the actual length
- // last else block: store to temp file and re-read
- final File tempFile = File.createTempFile("inputstream-", ".tmp");
- tempFile.deleteOnExit(); // Clean up after JVM exits
+ // we have to read all the stream to get the actual length
+ // last else block: store to temp file and re-read
+ final File tempFile = File.createTempFile("inputstream-", ".tmp");
Review Comment:
addressed in https://github.com/apache/jackrabbit-oak/pull/2688/commits/8f64b8a8e32ceb92024fde9998bdfba63220e5f3
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-12062 : fixed NPE while uploading metadata for AWS S3 [jackrabbit-oak]
Posted by "thomasmueller (via GitHub)" <gi...@apache.org>.
thomasmueller commented on code in PR #2688:
URL: https://github.com/apache/jackrabbit-oak/pull/2688#discussion_r2704545885
##########
oak-blob-cloud/src/main/java/org/apache/jackrabbit/oak/blob/cloud/s3/S3Backend.java:
##########
@@ -1334,42 +1334,38 @@ private static String getIdentifierName(String key) {
@NotNull
private AsyncRequestBody getRequestBody(final InputStream input, final ExecutorService executor,
final PutObjectRequest.Builder builder) throws IOException {
+ // for both AWS/GCP we need to know the length in advance, else it won't work.
final AsyncRequestBody body;
- if (Objects.equals(RemoteStorageMode.S3, properties.get(S3Constants.MODE))) {
- body = AsyncRequestBody.fromInputStream(input, null, executor);
+ final long length;
+ if (input instanceof FileInputStream) {
+ final FileInputStream fis = (FileInputStream) input;
+ // if the file is modified after opening, the size may not reflect the latest changes
+ length = fis.getChannel().size();
+ body = AsyncRequestBody.fromInputStream(input, length, executor);
+ } else if (input instanceof ByteArrayInputStream) {
+ length = input.available();
+ body = AsyncRequestBody.fromInputStream(input, length, executor);
+ } else if (input.markSupported()) {
+ // in case the inputStream supports mark & reset
+ input.mark(Integer.MAX_VALUE);
+ length = IOUtils.consume(input);
+ input.reset();
+ body = AsyncRequestBody.fromInputStream(input, length, executor);
} else {
- // for GCP we need to know the length in advance, else it won't work.
- final long length;
- if (input instanceof FileInputStream) {
- final FileInputStream fis = (FileInputStream) input;
- // if the file is modified after opening, the size may not reflect the latest changes
- length = fis.getChannel().size();
- body = AsyncRequestBody.fromInputStream(input, length, executor);
- } else if (input instanceof ByteArrayInputStream) {
- length = input.available();
- body = AsyncRequestBody.fromInputStream(input, length, executor);
- } else if (input.markSupported()) {
- // in case the inputStream supports mark & reset
- input.mark(Integer.MAX_VALUE);
- length = IOUtils.consume(input);
- input.reset();
- body = AsyncRequestBody.fromInputStream(input, length, executor);
- } else {
- // we have to read all the stream to get the actual length
- // last else block: store to temp file and re-read
- final File tempFile = File.createTempFile("inputstream-", ".tmp");
- tempFile.deleteOnExit(); // Clean up after JVM exits
+ // we have to read all the stream to get the actual length
+ // last else block: store to temp file and re-read
+ final File tempFile = File.createTempFile("inputstream-", ".tmp");
+ tempFile.deleteOnExit(); // Clean up after JVM exits
Review Comment:
This will delete on exit, but not delete after reading. I would delete after reading as well, so that in the normal case (where the process is not killed) the file is deleted. Otherwise we might have a large number of temp files, if the process is not terminated for a long time.
(This could cause out-of-disk space, and if the number of files is huge, also out-of-memory in kubernetes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-12062 : fixed NPE while uploading metadata for AWS S3 [jackrabbit-oak]
Posted by "rishabhdaim (via GitHub)" <gi...@apache.org>.
rishabhdaim commented on code in PR #2688:
URL: https://github.com/apache/jackrabbit-oak/pull/2688#discussion_r2706855516
##########
oak-blob-cloud/src/main/java/org/apache/jackrabbit/oak/blob/cloud/s3/S3Backend.java:
##########
@@ -536,6 +562,13 @@ public void addMetadataRecord(final InputStream input, final String name) throws
if (contextClassLoader != null) {
Thread.currentThread().setContextClassLoader(contextClassLoader);
}
+ if (tempFile != null) {
+ try {
+ Files.deleteIfExists(tempFile.toPath());
+ } catch (IOException e) {
+ LOG.warn("Failed to delete temp file {}", tempFile, e);
+ }
+ }
Review Comment:
@thomasmueller here I am calling `delete` inside `finally` block immediately after uploading.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-12062 : fixed NPE while uploading metadata for AWS S3 [jackrabbit-oak]
Posted by "reschke (via GitHub)" <gi...@apache.org>.
reschke commented on code in PR #2688:
URL: https://github.com/apache/jackrabbit-oak/pull/2688#discussion_r2704287567
##########
oak-blob-cloud/src/main/java/org/apache/jackrabbit/oak/blob/cloud/s3/S3Backend.java:
##########
@@ -1334,42 +1334,38 @@ private static String getIdentifierName(String key) {
@NotNull
private AsyncRequestBody getRequestBody(final InputStream input, final ExecutorService executor,
final PutObjectRequest.Builder builder) throws IOException {
+ // for both AWS/GCP we need to know the length in advance, else it won't work.
final AsyncRequestBody body;
- if (Objects.equals(RemoteStorageMode.S3, properties.get(S3Constants.MODE))) {
- body = AsyncRequestBody.fromInputStream(input, null, executor);
+ final long length;
+ if (input instanceof FileInputStream) {
+ final FileInputStream fis = (FileInputStream) input;
+ // if the file is modified after opening, the size may not reflect the latest changes
+ length = fis.getChannel().size();
+ body = AsyncRequestBody.fromInputStream(input, length, executor);
+ } else if (input instanceof ByteArrayInputStream) {
+ length = input.available();
+ body = AsyncRequestBody.fromInputStream(input, length, executor);
+ } else if (input.markSupported()) {
+ // in case the inputStream supports mark & reset
+ input.mark(Integer.MAX_VALUE);
+ length = IOUtils.consume(input);
+ input.reset();
+ body = AsyncRequestBody.fromInputStream(input, length, executor);
} else {
- // for GCP we need to know the length in advance, else it won't work.
- final long length;
- if (input instanceof FileInputStream) {
- final FileInputStream fis = (FileInputStream) input;
- // if the file is modified after opening, the size may not reflect the latest changes
- length = fis.getChannel().size();
- body = AsyncRequestBody.fromInputStream(input, length, executor);
- } else if (input instanceof ByteArrayInputStream) {
- length = input.available();
- body = AsyncRequestBody.fromInputStream(input, length, executor);
- } else if (input.markSupported()) {
- // in case the inputStream supports mark & reset
- input.mark(Integer.MAX_VALUE);
- length = IOUtils.consume(input);
- input.reset();
- body = AsyncRequestBody.fromInputStream(input, length, executor);
- } else {
- // we have to read all the stream to get the actual length
- // last else block: store to temp file and re-read
- final File tempFile = File.createTempFile("inputstream-", ".tmp");
- tempFile.deleteOnExit(); // Clean up after JVM exits
+ // we have to read all the stream to get the actual length
+ // last else block: store to temp file and re-read
+ final File tempFile = File.createTempFile("inputstream-", ".tmp");
Review Comment:
Prefix should show which class created the temp file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org