Add StreamMessage.timeout() #5761

sjy982 · 2024-06-13T10:15:19Z

Motivation:
Currently, the aggregate() and subscribe() methods of StreamMessage do not have the ability to set a timeout. This provides the ability to detect when a client or server has not responded for a period of time and handle it appropriately. Additionally, the timeout API can be used to detect idle streams by setting a timeout until the next message.

Modifications:

Added the TimeoutStreamMessage class

Wrap a StreamMessage to provide timeout functionality
You can set a timeout feature by adding the timeout() method to the StreamMessage interface.

Added the TimeoutSubscriber class

Schedule a timeout for each message.
StreamTimeoutMode allows you to set different timeout modes.

Added StreamTimeoutMode enumeration

Defines the UNTIL_FIRST, UNTIL_NEXT, and UNTIL_EOS modes.

Added a timeout method

Added the timeout method to the StreamMessage, HttpResponse, and HttpRequest interfaces to provide the ability to set a timeout.

Result:

Closes Add StreamMessage.timeout() #5744
This change adds timeout functionality to the aggregate() and subscribe() methods of StreamMessage and HTTP requests/responses.
This allows idle streams to be detected and handled appropriately.
You can use StreamTimeoutMode to set the timeout between the arrival of the first message, the arrival of the next message, or the end of the stream.

CLAassistant · 2024-06-13T10:15:34Z

All committers have signed the CLA.

github-actions · 2024-06-13T11:33:40Z

🔍 Build Scan® (commit: `24245a4`)

Job name	Status	Build Scan®
build-ubicloud-standard-8-jdk-8	✅	https://ge.armeria.dev/s/onrrsbmyxwv7s
build-ubicloud-standard-8-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/lo4aanlxcb2co
build-ubicloud-standard-8-jdk-17-min-java-17-coverage	❌ (failure)	https://ge.armeria.dev/s/us7irp3ponwji
build-ubicloud-standard-8-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/woix3jlaukvdy
build-ubicloud-standard-8-jdk-17-leak	✅	https://ge.armeria.dev/s/4iy736pszda4g
build-ubicloud-standard-8-jdk-11	✅	https://ge.armeria.dev/s/uddd2o3sck4ns
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/vyf5i26tmjg4o

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

Bue-von-hon · 2024-06-24T01:35:09Z

Some of the descriptions in the Modifications field contain Korean.

Added StreamTimeoutMode enumeration
UNTIL_FIRST, UNTIL_NEXT, UNTIL_EOS 모드를 정의합니다.

p.s. It's an interesting feature, so I'm watching with interest

sjy982 · 2024-06-24T13:32:55Z

Some of the descriptions in the Modifications field contain Korean.

Added StreamTimeoutMode enumeration
UNTIL_FIRST, UNTIL_NEXT, UNTIL_EOS 모드를 정의합니다.

p.s. It's an interesting feature, so I'm watching with interest

Thank you for your interest.

trustin

Thanks! Please make the build pass as well.

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

trustin · 2024-06-25T03:18:04Z

Last but not least, please sign the ICLA.

sjy982 · 2024-06-25T13:09:18Z

Last but not least, please sign the ICLA.

I checked, thank you for your review.

…ditions

jrhee17

Looks mostly done 👍 Left a couple of comments regarding edge cases

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

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

Unsubscribing due to a timeout does not guarantee synchronization with upstream. Therefore, we implemented an attempt attemptTerminate() method.

Lint error correction

minwoox

Looks great! Left a few suggestions. 👍

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

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

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

@throws

1. remove @throws from docs 2. add @UnstableApi with timeout method 3. public -> final 4. add @nullable to timeoutSubscriber 5. cancel() override (add cancelSchedule()) 6. lastontime nano -> lasteventtime nano 7. createTimeoutSchedule -> scheduleTimeout 8. call onError after canceling

minwoox

Still Looks good. 👍 Left a few more suggestions.

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

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

minwoox

I left a few more suggestions. 😉

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

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

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

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

2. add docs to StreamTimeoutMode parameter 3. use test code executor.get()

…and set the timeout schedule 2. change TimeoutSubscriber class to TimeoutStreamMessage's inner class

minwoox

Looks great! Thanks a lot, @sjy982! 😆

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

jrhee17

Left some nit comments, basically looks good 👍 👍 👍

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

core/src/main/java/com/linecorp/armeria/common/HttpRequest.java

…meoutMode` 2. Added leak prevention code when completed in onNext() - PooledObjects.close(t)

ikhoon

We need to revise and proofread Javadoc but the implementation looks look.

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

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

core/src/main/java/com/linecorp/armeria/common/HttpRequest.java

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

2. Add a StreamTimeoutException 3. Overriding WebSocket's timeout method 4. Remove cancelSchedule() from abort() 5. Set timeoutFuture to null to avoid calling .isCancelled() which may access a volatile field. 6. Modify boolean cancels to volatile boolean cancels

ikhoon

Nice work, @sjy982! 💯🚀

ikhoon added the new feature label Jun 14, 2024

trustin reviewed Jun 20, 2024

View reviewed changes

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

sjy982 requested a review from trustin June 24, 2024 13:22

trustin reviewed Jun 25, 2024

View reviewed changes

sjy982 added 9 commits June 26, 2024 00:27

Implement TimeoutStreamMessage for stream message timeout functionality

045afcc

feat: Implement TimeoutSubscriber for stream timeout handling

5818e6c

Add timeout method to StreamMessage

02cccc3

refactor: Timeout, streamTimeoutMode variable names changed

f47216e

feat: Add timeout method to HttpRequest

1d8a37f

feat: Add timeout method to HttpResponse

8cd5d4f

Refactor TimeoutSubscriber to use isCancelled() instead of null checks

926d53d

Add TimeoutStreamMessageTest

4d0e10f

refactor: Optimize timeout handling in TimeoutSubscriber

d1ea297

sjy982 force-pushed the feature/stream-message-timeout branch from 7c52cef to d1ea297 Compare June 25, 2024 15:37

refactor: deduplication, Runnable implementation, change boundary con…

318ebf3

…ditions

jrhee17 reviewed Jun 26, 2024

View reviewed changes

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

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

sjy982 added 2 commits June 28, 2024 01:14

Add timeout unscheduling logic to abort().

b095d33

Unsubscribing due to a timeout does not guarantee synchronization with upstream. Therefore, we implemented an attempt attemptTerminate() method.

Create Java Docs,

621e141

Lint error correction

minwoox reviewed Jul 2, 2024

View reviewed changes

sjy982 requested review from trustin, minwoox and jrhee17 July 2, 2024 09:32

minwoox reviewed Jul 3, 2024

View reviewed changes

minwoox marked this pull request as ready for review July 5, 2024 07:22

minwoox requested a review from ikhoon as a code owner July 5, 2024 07:22

minwoox reviewed Jul 5, 2024

View reviewed changes

sjy982 added 3 commits July 7, 2024 20:08

1. replace 2019 with 2024

45008d8

2. add docs to StreamTimeoutMode parameter 3. use test code executor.get()

1. call delegate.onSubscribe() in TimeoutSubscriber's onSubscribe(), …

e4c3916

…and set the timeout schedule 2. change TimeoutSubscriber class to TimeoutStreamMessage's inner class

lint

13228b3

sjy982 requested a review from minwoox July 8, 2024 11:25

minwoox approved these changes Jul 8, 2024

View reviewed changes

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

Remove blank lines

73adfff

sjy982 force-pushed the feature/stream-message-timeout branch from 251daf7 to 73adfff Compare July 8, 2024 13:20

jrhee17 approved these changes Jul 10, 2024

View reviewed changes

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

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

jrhee17 reviewed Jul 10, 2024

View reviewed changes

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

sjy982 added 2 commits July 10, 2024 23:48

1. Add @UnstableApi to HttpResponse, HttpRequest, and `StreamTi…

38b6eb9

…meoutMode` 2. Added leak prevention code when completed in onNext() - PooledObjects.close(t)

1. Added boolean canceled to prevent unnecessary scheduling.

566d725

sjy982 requested a review from jrhee17 July 11, 2024 07:47

ikhoon reviewed Jul 16, 2024

View reviewed changes

sjy982 added 3 commits July 17, 2024 19:54

1. Modify JavaDoc

eb8d450

2. Add a StreamTimeoutException 3. Overriding WebSocket's timeout method 4. Remove cancelSchedule() from abort() 5. Set timeoutFuture to null to avoid calling .isCancelled() which may access a volatile field. 6. Modify boolean cancels to volatile boolean cancels

lint pass

994ce96

lint pass

cdeec5c

sjy982 requested a review from ikhoon July 17, 2024 11:34

ikhoon added 3 commits July 25, 2024 18:13

javadoc

a95f279

Merge branch 'main' into pr-5761-sjy982-feature/stream-message-timeout

efb32b3

nullaway

2560092

ikhoon approved these changes Jul 25, 2024

View reviewed changes

ikhoon modified the milestones: 1.30.0, 1.31.0 Aug 1, 2024

github-actions bot added the Stale label Sep 1, 2024

Merge branch 'main' into feature/stream-message-timeout

24245a4

github-actions bot removed the Stale label Nov 7, 2024

jrhee17 merged commit 1bb781a into line:main Nov 7, 2024
13 of 14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add StreamMessage.timeout() #5761

Add StreamMessage.timeout() #5761

sjy982 commented Jun 13, 2024 •

edited

Loading

CLAassistant commented Jun 13, 2024 •

edited

Loading

github-actions bot commented Jun 13, 2024 •

edited

Loading

Bue-von-hon commented Jun 24, 2024

sjy982 commented Jun 24, 2024

trustin left a comment

trustin commented Jun 25, 2024

sjy982 commented Jun 25, 2024

jrhee17 left a comment

minwoox left a comment

minwoox left a comment

minwoox left a comment

minwoox left a comment

jrhee17 left a comment

ikhoon left a comment

ikhoon left a comment

Add StreamMessage.timeout() #5761

Add StreamMessage.timeout() #5761

Conversation

sjy982 commented Jun 13, 2024 • edited Loading

CLAassistant commented Jun 13, 2024 • edited Loading

github-actions bot commented Jun 13, 2024 • edited Loading

🔍 Build Scan® (commit: 24245a4)

Bue-von-hon commented Jun 24, 2024

sjy982 commented Jun 24, 2024

trustin left a comment

Choose a reason for hiding this comment

trustin commented Jun 25, 2024

sjy982 commented Jun 25, 2024

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

minwoox left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

sjy982 commented Jun 13, 2024 •

edited

Loading

CLAassistant commented Jun 13, 2024 •

edited

Loading

github-actions bot commented Jun 13, 2024 •

edited

Loading

🔍 Build Scan® (commit: `24245a4`)