-
Notifications
You must be signed in to change notification settings - Fork 4k
api: Implement custom events framework in gRPC-Java server #12980
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
45b5af1
e37245c
2636ebb
5caf237
2158b97
605cc03
dd549f3
d5f0182
542f840
958fddc
feeab1e
b9e1e2b
e832422
48aee63
1b8a230
bdfdafe
b63b4a1
91ec839
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -254,6 +254,17 @@ public MethodDescriptor<ReqT, RespT> getMethodDescriptor() { | |
| return method; | ||
| } | ||
|
|
||
| @Override | ||
| public void triggerEvent(Object event) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need a cancellation and close check here? other methods seem to have it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A check in
The
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am less worried about races since we can fix and address them and more worried about the expected behavior. Right now it seems like we don't check cancellation or closure status here when triggering operations which may be okay if our interface contract is "you should not call after cancellation" . If our contract allows or specifies the behavior after cancellation , I'd assume we check and enforce it here in the implementation.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
so that implies that a
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unresolving. While looking at it, I realized that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The thread is using a thread-local variable, it is not even meant for cross thread visibility. So it doesn't fit into the definition you are quoting at all.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might not be on the same page here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is not thread-local. What I really meant was that the only scenario where it helps is intra-thread offering a fast path. Where it is visible to other threads (like a volatile), it still does nothing about preventing races.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's do this. Let's try to change the test so that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TSAN will flag it as a race, I'm not contesting that. But there is a reason TSAN suppressions exist (ex. CL/967859705) for developers to indicate intentionally allowed races that have no adverse impact.
sauravzg marked this conversation as resolved.
|
||
| if (closeCalled) { | ||
| return; | ||
| } | ||
| try (TaskCloseable ignore = PerfMark.traceTask("ServerCall.triggerEvent")) { | ||
| PerfMark.attachTag(tag); | ||
| stream.triggerEvent(event); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public SecurityLevel getSecurityLevel() { | ||
| final Attributes attributes = getAttributes(); | ||
|
|
@@ -395,5 +406,16 @@ public void onReady() { | |
| listener.onReady(); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void triggerEvent(Object event) { | ||
| try (TaskCloseable ignore = PerfMark.traceTask("ServerStreamListener.triggerEvent")) { | ||
| PerfMark.attachTag(call.tag); | ||
| if (call.cancelled) { | ||
| return; | ||
| } | ||
| listener.onEvent(event); | ||
| } | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.