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.

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

Fixes SubscribeToLogs example to work with authenticated channel #3400

Merged
merged 6 commits into from
Feb 3, 2023

Conversation

devinrsmith
Copy link
Member

No description provided.

@devinrsmith devinrsmith added bug Something isn't working NoDocumentationNeeded java-client NoReleaseNotesNeeded No release notes are needed. labels Feb 2, 2023
@devinrsmith devinrsmith added this to the Jan 2023 milestone Feb 2, 2023
@devinrsmith devinrsmith self-assigned this Feb 2, 2023
@Override
public void onNext(LogSubscriptionData value) {
latch.countDown();
++count;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is likely to be invoked from another thread. You need to make count, and error, volatile to avoid a racy test. (Or synchronize onNext / onError.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the semantics provided by the CountDownLatch are good enough - I can refactor a bit to get rid of count; and, onError / onComplete are guaranteed to only be called once, and visibility of error should be guarded by done. I'll re-push, let me know what you think.

@devinrsmith devinrsmith merged commit fe5f36b into deephaven:main Feb 3, 2023
@devinrsmith devinrsmith deleted the deephaven-channel-mixin branch February 3, 2023 20:22
@github-actions github-actions bot locked and limited conversation to collaborators Feb 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working java-client NoDocumentationNeeded NoReleaseNotesNeeded No release notes are needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants