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

Subscriber Authentication #14

Closed
wants to merge 1 commit into from
Closed

Subscriber Authentication #14

wants to merge 1 commit into from

Conversation

rexlManu
Copy link

@rexlManu rexlManu commented Jul 3, 2022

I tried to use the subscriber client with a redis instance thats password-protected and found out by #8 that authentication is missing.

@sonarcloud
Copy link

sonarcloud bot commented Jul 3, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@TomTruyen
Copy link

@crackthecodeabhi can you review this PR and make a patch release for it? I'm in desperate need for this for my project and Kreds would be perfect (for my current use case) if this feature was added. Thanks in advance :)

@rexlManu
Copy link
Author

@crackthecodeabhi can you review this PR and make a patch release for it? I'm in desperate need for this for my project and Kreds would be perfect (for my current use case) if this feature was added. Thanks in advance :)

You can download my fork and install it in your local maven repository, if you need it urgent. I think the author doesn't maintain it further.

@TomTruyen
Copy link

@crackthecodeabhi can you review this PR and make a patch release for it? I'm in desperate need for this for my project and Kreds would be perfect (for my current use case) if this feature was added. Thanks in advance :)

You can download my fork and install it in your local maven repository, if you need it urgent. I think the author doesn't maintain it further.

I have just been in contact with the author and he said the following:
Yes, I will review and add some test cases to it before making a release. Should be done in a week.

But I will probably do what you said until the new release is available

@crackthecodeabhi
Copy link
Owner

@rexlManu @TomTruyen will be reviewing and merging the issue ASAP. been busy lately. Give me sometime.

@JefCodes
Copy link

@rexlManu @TomTruyen will be reviewing and merging the issue ASAP. been busy lately. Give me sometime.

Is there an ETA on this? The PubSub functionality is almost useless without it... I am stuck on project because of this issue

@crackthecodeabhi
Copy link
Owner

@rexlManu @TomTruyen will be reviewing and merging the issue ASAP. been busy lately. Give me sometime.

Is there an ETA on this? The PubSub functionality is almost useless without it... I am stuck on project because of this issue

There needs to be some work done on this PR, the current PR will cause a dead lock since the reader coroutine is not pre-empted before writing on the connection.

Will update as soon i fix it and run some tests.

@TomTruyen
Copy link

@rexlManu @TomTruyen will be reviewing and merging the issue ASAP. been busy lately. Give me sometime.

Is there an ETA on this? The PubSub functionality is almost useless without it... I am stuck on project because of this issue

There needs to be some work done on this PR, the current PR will cause a dead lock since the reader coroutine is not pre-empted before writing on the connection.

Will update as soon i fix it and run some tests.

I didn't get a dead lock when using @rexlManu his version. Does the dead lock only happen in specific scenario's?

@rexlManu
Copy link
Author

I'm currently using my version pretty much and didn't run in any issues. Maybe share a example where it occurs.

@crackthecodeabhi
Copy link
Owner

crackthecodeabhi commented Aug 18, 2022

@rexlManu the subscriber coroutine suspends [waititng to read messages from redis] after it locks the mutex on the Konnection.
the Connection command interface which has now been added as implements to Subscriber Client,
will not preempt this coroutine, before it takes a lock on the that connection mutex.

if the user subscribes to a channel, and then tries to execute any connection command, it will block indefinitely.

Hence, a clean approach would be to , accept optional auth parameters while building the subscriber client, and authenticate against redis before doing anything else, this ensure all other commands work as usual with no change.

also correction on my part, it won't be a dead lock but unintended blocking of the coroutine indefinitely.

@crackthecodeabhi
Copy link
Owner

@rexlManu @TomTruyen will be reviewing and merging the issue ASAP. been busy lately. Give me sometime.

Is there an ETA on this? The PubSub functionality is almost useless without it... I am stuck on project because of this issue

There needs to be some work done on this PR, the current PR will cause a dead lock since the reader coroutine is not pre-empted before writing on the connection.
Will update as soon i fix it and run some tests.

I didn't get a dead lock when using @rexlManu his version. Does the dead lock only happen in specific scenario's?

After calling the subscribe command on the subscriber client, call any other connection command, the coroutine will block

@rexlManu rexlManu closed this by deleting the head repository Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants