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.

Sign up for GitHub

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

Jump to bottom

Do not ignore the result of StreamMessage.tryWrite() in GraphqlWSSubProtocol #5542

Open
minwoox opened this issue Mar 28, 2024 · 6 comments · May be fixed by #5557
Open

Do not ignore the result of StreamMessage.tryWrite() in GraphqlWSSubProtocol #5542

minwoox opened this issue Mar 28, 2024 · 6 comments · May be fixed by #5557
Labels
defect

Comments

@minwoox
Copy link
Member

minwoox commented Mar 28, 2024

Currently. we ignore the result of StreamMessage.tryWrite() in several places in GraphqlWSSubProtocol:

armeria/graphql/src/main/java/com/linecorp/armeria/server/graphql/GraphqlWSSubProtocol.java

Line 355 in d9703ed

out.tryWrite(event);

We shouldn't ignore it. If it returns false, we should cancel the publisher and cleanup the resource.
@minwoox minwoox added sprint Issues for OSS Sprint participants defect labels Mar 28, 2024
@pushrsp
Copy link

pushrsp commented Apr 1, 2024

@minwoox , can i work on it?

@minwoox
Copy link
Member Author

minwoox commented Apr 1, 2024

Hi, @pushrsp! Yeah, you can have this. Thanks!

@minwoox minwoox removed the sprint Issues for OSS Sprint participants label Apr 1, 2024
@pushrsp
Copy link

pushrsp commented Apr 1, 2024

Hi, @pushrsp! Yeah, you can have this. Thanks!

@minwoox, I'm gonna try to fix it like this below:
Screenshot 2024-04-01 at 4 26 21 PM

Does it look good to you?

@minwoox
Copy link
Member Author

minwoox commented Apr 1, 2024

We also have to cancel the publisher that emits the data.
I highly recommend you read some articles about Reactive Streams. e.g. https://engineering.linecorp.com/en/blog/reactive-streams-armeria-1

@pushrsp
Copy link

pushrsp commented Apr 2, 2024

As far as I know, If tryWrite is failed, it means that WebSocketWriter is already closed.
So, What kind of exception should be thrown?
Or just use write instead of tryWrite because in try, it throws ClosedStreamException if WebSocketWriter is closed, so let exception handler in ExecutionResultSubscriber.onNext() pick ClosedStreamException up and cleanup the resource.

@minwoox
Copy link
Member Author

minwoox commented Apr 2, 2024

If tryWrite is failed, it means that WebSocketWriter is already closed.

That is correct. 👍
Even though the writer is closed, the publisher still might be emitting the data.

armeria/graphql/src/main/java/com/linecorp/armeria/server/graphql/GraphqlWSSubProtocol.java

Line 274 in d9703ed

streamMessage.subscribe(executionResultSubscriber, ctx.eventLoop());

So if tryWrite returns false, we should cancel the publisher not to emit the data anymore.

armeria/graphql/src/main/java/com/linecorp/armeria/server/graphql/GraphqlWSSubProtocol.java

Line 244 in d9703ed

writeNext(out, operationId, executionResult);

@pushrsp pushrsp linked a pull request Apr 2, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
defect
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add exception when tryWrite returns false pushrsp/armeria
2 participants
@minwoox @pushrsp