Add `StreamMessage.of(inputStream)` #4703

injae-kim · 2023-02-25T16:48:41Z

Related Issue #3937

Motivation:

static StreamMessage<HttpData> of(InputStream inputStream) {...}

It would be nice if we provide a way to convert an InputStream to a StreamMessage that splits the binary by the bufferSize.

Modifications:

Add StreamMessage.of(inputStream)
Add InputStreamStreamMessage and builder

Result:

You can now use StreamMessage.of(inputStream) to convert an InputStream to StreamMessaeg<HttpData> that splits the binary by the bufferSize
Close Add StreamMessage.of(inputStream, bufferSize) #3937 issue

core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessageBuilder.java

core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java

core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessageBuilder.java

core/src/main/java/com/linecorp/armeria/common/stream/StreamMessage.java

ikhoon · 2023-02-27T05:16:50Z

@injae-kim Why don't you send a PR to https://github.com/softwaremill/tapir in which I left a TODO for this issue when a new version is released? 😉
https://github.com/softwaremill/tapir/blob/master/server/armeria-server/src/main/scala/sttp/tapir/server/armeria/ArmeriaToResponseBody.scala#L52-L53

ikhoon

Overall, looks good.

core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java

injae-kim · 2023-02-27T17:24:15Z

// TODO(ikhoon): Add StreamMessage.of(InputStream)
Why don't you send a PR to https://github.com/softwaremill/tapir in which I left a TODO for this issue when a new version is released? 😉

oh~ after this PR is merged and released with new version, I'll also send PR to tapir to remove TODO~!

trustin · 2023-02-28T09:15:21Z

core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java

+        if (this.blockingTaskExecutor != null) {
+            blockingTaskExecutor = this.blockingTaskExecutor;
+        } else {
+            final ServiceRequestContext serviceRequestContext = ServiceRequestContext.currentOrNull();
+            if (serviceRequestContext != null) {
+                blockingTaskExecutor = serviceRequestContext.blockingTaskExecutor();
+            } else {
+                blockingTaskExecutor = CommonPools.blockingTaskExecutor();
+            }
+        }


I see some duplications between this class and PathStreamMessage. Can we deduplicate as much as possible, potentially introducing a common supertype?

Can we deduplicate as much as possible, potentially introducing a common supertype?

SGTM! but may I handle it with another issue & PR?
Cause I think we need some discussion about this issue 🙇

If you want to handle it in a separate issue, it is okay to send a clean-up PR.

I'll send clean-up PR after this PR is merged~!

core/src/test/java/com/linecorp/armeria/common/stream/InputStreamStreamMessageTest.java

core/src/test/java/com/linecorp/armeria/common/stream/InputStreamStreamMessageTckTest.java

jrhee17

In terms of correctness InputStreamStreamMessage looks good 👍 I think this PR is almost done! 🚀 Left only nit comments 🙇

core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java

core/src/main/java/com/linecorp/armeria/common/stream/AbstractByteStreamMessageBuilder.java

jrhee17

The changeset looks good to me (bar the comment #4703 (comment) which I think you'll be following up on) 😄
Thanks @injae-kim 👍 🙇 👍

core/src/main/java/com/linecorp/armeria/common/stream/StreamMessage.java

minwoox

Thanks! Left some suggestions and a question. 😄

core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessageBuilder.java

core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java

core/src/main/java/com/linecorp/armeria/common/stream/StreamMessage.java

core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java

minwoox

Thanks a lot for adding this nice feature! 👍 🙇 👍

core/src/main/java/com/linecorp/armeria/common/stream/PathStreamMessageBuilder.java

core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java

trustin

👍 once all others comments are addressed.

core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java

…eamStreamMessage.java

core/src/main/java/com/linecorp/armeria/common/stream/StreamMessage.java

core/src/test/java/com/linecorp/armeria/common/stream/InputStreamStreamMessageTest.java

…ssage.java

…eamStreamMessageTest.java

…ssage.java

ikhoon

Awesome work! 🚀💯

injae-kim requested review from ikhoon, jrhee17, minwoox and trustin as code owners February 25, 2023 16:48

minwoox added the new feature label Feb 25, 2023

minwoox added this to the 1.23.0 milestone Feb 25, 2023

Add StreamMessage.of(inputStream)

7c7de9c

injae-kim force-pushed the add-stream-message-input-stream branch from a68980d to 7c7de9c Compare February 25, 2023 17:10

injae-kim commented Feb 27, 2023

View reviewed changes

ikhoon reviewed Feb 27, 2023

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java Outdated Show resolved Hide resolved

core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java Outdated Show resolved Hide resolved

injae-kim added 3 commits February 28, 2023 01:50

Ensure close(cause) is invoked when aborted before subscribed

f82bdf3

Address comment on publishDownstream()

99c85d0

Introduce InternalStreamMessageUtil#DEFAULT_FILE_BUFFER_SIZE value

26506bc

trustin reviewed Feb 28, 2023

View reviewed changes

Add InputStreamStreamMessageTckTest

212d0bb

injae-kim commented Mar 2, 2023

View reviewed changes

core/src/test/java/com/linecorp/armeria/common/stream/InputStreamStreamMessageTckTest.java Show resolved Hide resolved

jrhee17 reviewed Mar 7, 2023

View reviewed changes

Address @jrhee17 comments

cbd0ed3

jrhee17 approved these changes Mar 8, 2023

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/stream/StreamMessage.java Show resolved Hide resolved

revert reviews from @jrhee17

b485b11

minwoox reviewed Mar 8, 2023

View reviewed changes

ikhoon reviewed Mar 8, 2023

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java Outdated Show resolved Hide resolved

core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java Show resolved Hide resolved

ikhoon reviewed Mar 8, 2023

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java Outdated Show resolved Hide resolved

injae-kim added 4 commits March 9, 2023 19:13

Move alloc() down to PathStreamMessageBuilder

332cddc

Address @minwoox comments

33989c2

Address @ikhoon comments

ab8c181

Fix checkStyle

7d7576d

minwoox approved these changes Mar 9, 2023

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/stream/PathStreamMessageBuilder.java Outdated Show resolved Hide resolved

core/src/main/java/com/linecorp/armeria/common/stream/PathStreamMessageBuilder.java Outdated Show resolved Hide resolved

injae-kim added 2 commits March 9, 2023 23:59

Remove unnecessary alloc getter

9631349

Handle concurrency issue on readBytes() by reading boolean flag

0fb02c6

injae-kim commented Mar 9, 2023

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java Show resolved Hide resolved

trustin reviewed Mar 10, 2023

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java Outdated Show resolved Hide resolved

core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java Outdated Show resolved Hide resolved

trustin reviewed Mar 10, 2023

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java Outdated Show resolved Hide resolved

Address @trustin comments

5898efa

injae-kim force-pushed the add-stream-message-input-stream branch from 796198e to 5898efa Compare March 10, 2023 08:45

trustin approved these changes Mar 10, 2023

View reviewed changes

ikhoon reviewed Mar 13, 2023

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java Outdated Show resolved Hide resolved

core/src/main/java/com/linecorp/armeria/common/stream/InputStreamStreamMessage.java Outdated Show resolved Hide resolved

ikhoon added 2 commits March 13, 2023 12:59

Update core/src/main/java/com/linecorp/armeria/common/stream/InputStr…

0b7e91f

…eamStreamMessage.java

Update core/src/main/java/com/linecorp/armeria/common/stream/InputStr…

8c2ee10

…eamStreamMessage.java

ikhoon reviewed Mar 13, 2023

View reviewed changes

ikhoon added 4 commits March 13, 2023 13:12

Update core/src/main/java/com/linecorp/armeria/common/stream/StreamMe…

547ccf0

…ssage.java

Update core/src/test/java/com/linecorp/armeria/common/stream/InputStr…

aeb27ac

…eamStreamMessageTest.java

Update core/src/main/java/com/linecorp/armeria/common/stream/StreamMe…

677f517

…ssage.java

Add @UnstableApi

62b8f42

ikhoon approved these changes Mar 13, 2023

View reviewed changes

ikhoon merged commit 0d94d0d into line:master Mar 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `StreamMessage.of(inputStream)` #4703

Add `StreamMessage.of(inputStream)` #4703

injae-kim commented Feb 25, 2023

ikhoon commented Feb 27, 2023 •

edited

Loading

ikhoon left a comment

injae-kim commented Feb 27, 2023 •

edited

Loading

trustin Feb 28, 2023

injae-kim Mar 2, 2023

ikhoon Mar 10, 2023

injae-kim Mar 10, 2023

jrhee17 left a comment

jrhee17 left a comment

minwoox left a comment

minwoox left a comment

trustin left a comment

ikhoon left a comment

Add StreamMessage.of(inputStream) #4703

Add StreamMessage.of(inputStream) #4703

Conversation

injae-kim commented Feb 25, 2023

Motivation:

Modifications:

Result:

ikhoon commented Feb 27, 2023 • edited Loading

ikhoon left a comment

Choose a reason for hiding this comment

injae-kim commented Feb 27, 2023 • edited Loading

trustin Feb 28, 2023

Choose a reason for hiding this comment

injae-kim Mar 2, 2023

Choose a reason for hiding this comment

ikhoon Mar 10, 2023

Choose a reason for hiding this comment

injae-kim Mar 10, 2023

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

Add `StreamMessage.of(inputStream)` #4703

Add `StreamMessage.of(inputStream)` #4703

ikhoon commented Feb 27, 2023 •

edited

Loading

injae-kim commented Feb 27, 2023 •

edited

Loading