You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "maxxedev (via GitHub)" <gi...@apache.org> on 2023/04/11 07:06:39 UTC
[GitHub] [commons-io] maxxedev opened a new pull request, #447: Add blocking read mode to QueueInputStream.
maxxedev opened a new pull request, #447:
URL: https://github.com/apache/commons-io/pull/447
Add blocking read mode to QueueInputStream.
To more closely mimic InputStream::read behavior, add blocking read mode to QueueInputStream that will wait until a queue element becomes available.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a diff in pull request #447: Add blocking read mode to QueueInputStream.
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #447:
URL: https://github.com/apache/commons-io/pull/447#discussion_r1162907587
##########
src/main/java/org/apache/commons/io/input/QueueInputStream.java:
##########
@@ -86,12 +99,24 @@ public QueueOutputStream newQueueOutputStream() {
}
/**
- * Reads and returns a single byte.
+ * Reads and returns a single byte. In blocking read mode, the method will wait if necessary until a queue element
+ * becomes available. In non-blocking read mode, the method will return -1 immediately if the queue is empty.
*
* @return either the byte read or {@code -1} if the end of the stream has been reached
+ * @throws InterruptedIOException if the thread is interrupted while reading from the queue.
*/
@Override
- public int read() {
+ public int read() throws InterruptedIOException {
Review Comment:
-1; I'm not a fan of this style because it is the opposite of OO: For each char, we test and execute one branch or another _based on state that never changes_, which tells me this should be implemented either as a subclass or pluggable strategy (with a lambda for example).
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #447: Add blocking read mode to QueueInputStream.
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #447:
URL: https://github.com/apache/commons-io/pull/447#issuecomment-1514015259
> > @maxxedev See git master where I took a different approach. [...]
>
> Thanks. I will look into improving the tests
You can rebase this PR, or close it and create a new one. Up to you ;-)
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] maxxedev commented on pull request #447: Add blocking read mode to QueueInputStream.
Posted by "maxxedev (via GitHub)" <gi...@apache.org>.
maxxedev commented on PR #447:
URL: https://github.com/apache/commons-io/pull/447#issuecomment-1517539425
Thinking more about this, there might be a way to keep it simple and still achieve the same result.
I opened a new PR. see [https://github.com/apache/commons-io/pull/452 ](https://github.com/apache/commons-io/pull/452)
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory closed pull request #447: Add blocking read mode to QueueInputStream.
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory closed pull request #447: Add blocking read mode to QueueInputStream.
URL: https://github.com/apache/commons-io/pull/447
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] maxxedev commented on pull request #447: Add blocking read mode to QueueInputStream.
Posted by "maxxedev (via GitHub)" <gi...@apache.org>.
maxxedev commented on PR #447:
URL: https://github.com/apache/commons-io/pull/447#issuecomment-1510669139
> @maxxedev See git master where I took a different approach. [...]
Thanks. I will look into improving the tests
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #447: Add blocking read mode to QueueInputStream.
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #447:
URL: https://github.com/apache/commons-io/pull/447#issuecomment-1518662360
Closing: PR #452 merged
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] maxxedev commented on a diff in pull request #447: Add blocking read mode to QueueInputStream.
Posted by "maxxedev (via GitHub)" <gi...@apache.org>.
maxxedev commented on code in PR #447:
URL: https://github.com/apache/commons-io/pull/447#discussion_r1163450815
##########
src/main/java/org/apache/commons/io/input/QueueInputStream.java:
##########
@@ -86,12 +99,24 @@ public QueueOutputStream newQueueOutputStream() {
}
/**
- * Reads and returns a single byte.
+ * Reads and returns a single byte. In blocking read mode, the method will wait if necessary until a queue element
+ * becomes available. In non-blocking read mode, the method will return -1 immediately if the queue is empty.
*
* @return either the byte read or {@code -1} if the end of the stream has been reached
+ * @throws InterruptedIOException if the thread is interrupted while reading from the queue.
*/
@Override
- public int read() {
+ public int read() throws InterruptedIOException {
Review Comment:
Good point. I created a subclass.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] maxxedev commented on a diff in pull request #447: Add blocking read mode to QueueInputStream.
Posted by "maxxedev (via GitHub)" <gi...@apache.org>.
maxxedev commented on code in PR #447:
URL: https://github.com/apache/commons-io/pull/447#discussion_r1168132133
##########
src/main/java/org/apache/commons/io/input/QueueInputStream.java:
##########
@@ -88,12 +111,32 @@ public QueueOutputStream newQueueOutputStream() {
/**
* Reads and returns a single byte.
*
- * @return either the byte read or {@code -1} if the end of the stream has been reached
+ * @return either the byte read or {@code -1} immediately if the queue is empty.
*/
@Override
public int read() {
final Integer value = blockingQueue.poll();
return value == null ? EOF : 0xFF & value;
}
+ private static class BlockingQueueInputStream extends QueueInputStream {
+
+ public BlockingQueueInputStream(final BlockingQueue<Integer> blockingQueue) {
+ super(blockingQueue);
+ }
+
+ @Override
+ public int read() {
+ try {
+ return blockingQueue.take();
+ } catch (final InterruptedException e) {
+ Thread.currentThread().interrupt();
+ final InterruptedIOException ioException = new InterruptedIOException();
Review Comment:
UncheckedIOException constructor requires a IOException.
InterruptedException does not extend IOException
InterruptedIOException extends IOException
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] codecov-commenter commented on pull request #447: Add blocking read mode to QueueInputStream.
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #447:
URL: https://github.com/apache/commons-io/pull/447#issuecomment-1502855110
## [Codecov](https://codecov.io/gh/apache/commons-io/pull/447?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#447](https://codecov.io/gh/apache/commons-io/pull/447?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (694da94) into [master](https://codecov.io/gh/apache/commons-io/commit/459431e0ed005e782b2ed147cda9cb8c6ba05357?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (459431e) will **decrease** coverage by `0.02%`.
> The diff coverage is `100.00%`.
```diff
@@ Coverage Diff @@
## master #447 +/- ##
============================================
- Coverage 84.77% 84.75% -0.02%
+ Complexity 3232 3231 -1
============================================
Files 218 218
Lines 7653 7663 +10
Branches 945 945
============================================
+ Hits 6488 6495 +7
- Misses 921 922 +1
- Partials 244 246 +2
```
| [Impacted Files](https://codecov.io/gh/apache/commons-io/pull/447?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../org/apache/commons/io/input/QueueInputStream.java](https://codecov.io/gh/apache/commons-io/pull/447?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvaW8vaW5wdXQvUXVldWVJbnB1dFN0cmVhbS5qYXZh) | `100.00% <100.00%> (ø)` | |
... and [2 files with indirect coverage changes](https://codecov.io/gh/apache/commons-io/pull/447/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: notifications-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #447: Add blocking read mode to QueueInputStream.
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #447:
URL: https://github.com/apache/commons-io/pull/447#issuecomment-1510656095
@maxxedev See git master where I took a different approach. What's next to do is a test for the new "Taking" class. I think it might make sense to have the test in a subclass or a completely separate class for clarity. The refactored test class makes it much harder to understand what we are testing and how (IMO).
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a diff in pull request #447: Add blocking read mode to QueueInputStream.
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #447:
URL: https://github.com/apache/commons-io/pull/447#discussion_r1168005091
##########
src/main/java/org/apache/commons/io/input/QueueInputStream.java:
##########
@@ -88,12 +111,32 @@ public QueueOutputStream newQueueOutputStream() {
/**
* Reads and returns a single byte.
*
- * @return either the byte read or {@code -1} if the end of the stream has been reached
+ * @return either the byte read or {@code -1} immediately if the queue is empty.
*/
@Override
public int read() {
final Integer value = blockingQueue.poll();
return value == null ? EOF : 0xFF & value;
}
+ private static class BlockingQueueInputStream extends QueueInputStream {
+
+ public BlockingQueueInputStream(final BlockingQueue<Integer> blockingQueue) {
+ super(blockingQueue);
+ }
+
+ @Override
+ public int read() {
+ try {
+ return blockingQueue.take();
+ } catch (final InterruptedException e) {
+ Thread.currentThread().interrupt();
+ final InterruptedIOException ioException = new InterruptedIOException();
Review Comment:
@maxxedev
This seems unusual, why not simply use `throw new UncheckedIOException(e)`?
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a diff in pull request #447: Add blocking read mode to QueueInputStream.
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #447:
URL: https://github.com/apache/commons-io/pull/447#discussion_r1168007000
##########
src/test/java/org/apache/commons/io/input/QueueInputStreamTest.java:
##########
@@ -17,24 +17,29 @@
package org.apache.commons.io.input;
import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.apache.commons.io.input.QueueInputStream.createBlockingQueueInputStream;
Review Comment:
@maxxedev
Only use static imports for JUnit assertions, otherwise, it's too confusing as to who does what IMO.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a diff in pull request #447: Add blocking read mode to QueueInputStream.
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #447:
URL: https://github.com/apache/commons-io/pull/447#discussion_r1162907587
##########
src/main/java/org/apache/commons/io/input/QueueInputStream.java:
##########
@@ -86,12 +99,24 @@ public QueueOutputStream newQueueOutputStream() {
}
/**
- * Reads and returns a single byte.
+ * Reads and returns a single byte. In blocking read mode, the method will wait if necessary until a queue element
+ * becomes available. In non-blocking read mode, the method will return -1 immediately if the queue is empty.
*
* @return either the byte read or {@code -1} if the end of the stream has been reached
+ * @throws InterruptedIOException if the thread is interrupted while reading from the queue.
*/
@Override
- public int read() {
+ public int read() throws InterruptedIOException {
Review Comment:
-1; I'm not a fan of this style because it is the opposite of OO: For each char, we test and execute one branch or another _based on state that never changes_, which tells me this should be implemented either as a subclass or pluggable strategy (with a lambda for example). IOW, this could likely be best as a separate class. Thoughts?
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a diff in pull request #447: Add blocking read mode to QueueInputStream.
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #447:
URL: https://github.com/apache/commons-io/pull/447#discussion_r1168005091
##########
src/main/java/org/apache/commons/io/input/QueueInputStream.java:
##########
@@ -88,12 +111,32 @@ public QueueOutputStream newQueueOutputStream() {
/**
* Reads and returns a single byte.
*
- * @return either the byte read or {@code -1} if the end of the stream has been reached
+ * @return either the byte read or {@code -1} immediately if the queue is empty.
*/
@Override
public int read() {
final Integer value = blockingQueue.poll();
return value == null ? EOF : 0xFF & value;
}
+ private static class BlockingQueueInputStream extends QueueInputStream {
+
+ public BlockingQueueInputStream(final BlockingQueue<Integer> blockingQueue) {
+ super(blockingQueue);
+ }
+
+ @Override
+ public int read() {
+ try {
+ return blockingQueue.take();
+ } catch (final InterruptedException e) {
+ Thread.currentThread().interrupt();
+ final InterruptedIOException ioException = new InterruptedIOException();
Review Comment:
@maxxedev
This seems unusual, why not simply use `new InterruptedIOException(e)` or even simpler `throw new UncheckedIOException(e)`?
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a diff in pull request #447: Add blocking read mode to QueueInputStream.
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #447:
URL: https://github.com/apache/commons-io/pull/447#discussion_r1168009124
##########
src/main/java/org/apache/commons/io/input/QueueInputStream.java:
##########
@@ -57,24 +59,45 @@
*/
public class QueueInputStream extends InputStream {
- private final BlockingQueue<Integer> blockingQueue;
+ final BlockingQueue<Integer> blockingQueue;
/**
- * Constructs a new instance with no limit to its internal buffer size.
+ * Constructs a new instance with no limit to its internal queue size
*/
public QueueInputStream() {
this(new LinkedBlockingQueue<>());
}
/**
- * Constructs a new instance with given buffer
+ * Constructs a new instance with given queue
*
* @param blockingQueue backing queue for the stream
*/
public QueueInputStream(final BlockingQueue<Integer> blockingQueue) {
this.blockingQueue = Objects.requireNonNull(blockingQueue, "blockingQueue");
}
+ /**
Review Comment:
I'd rather not have these factory methods, if we need something to build, let's use a Builder.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on a diff in pull request #447: Add blocking read mode to QueueInputStream.
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #447:
URL: https://github.com/apache/commons-io/pull/447#discussion_r1168065679
##########
src/main/java/org/apache/commons/io/input/QueueInputStream.java:
##########
@@ -57,24 +59,45 @@
*/
public class QueueInputStream extends InputStream {
- private final BlockingQueue<Integer> blockingQueue;
+ final BlockingQueue<Integer> blockingQueue;
/**
- * Constructs a new instance with no limit to its internal buffer size.
+ * Constructs a new instance with no limit to its internal queue size
*/
public QueueInputStream() {
this(new LinkedBlockingQueue<>());
}
/**
- * Constructs a new instance with given buffer
+ * Constructs a new instance with given queue
*
* @param blockingQueue backing queue for the stream
*/
public QueueInputStream(final BlockingQueue<Integer> blockingQueue) {
this.blockingQueue = Objects.requireNonNull(blockingQueue, "blockingQueue");
}
+ /**
Review Comment:
@maxxedev Hm, let me try something and get back to you...
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org