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

Added method to check subscribers #171

Conversation

sudeeptarlekar
Copy link

@sudeeptarlekar sudeeptarlekar commented Sep 20, 2020

resolves #169

Added method for checking is subscription is present or not for client

@sudeeptarlekar sudeeptarlekar marked this pull request as draft September 20, 2020 16:13
@sudeeptarlekar
Copy link
Author

Still working on specs

@sudeeptarlekar sudeeptarlekar marked this pull request as ready for review September 21, 2020 05:25
@sudeeptarlekar
Copy link
Author

@gmallard can I get a review?

@sudeeptarlekar
Copy link
Author

@gmallard any update on this?

@gmallard
Copy link

Running this:

https://gist.github.com/gmallard/be13b2ea80150c897768e22633fd2e39

gets me:

Client Connect complete
Client start puts
Sent: message payload: 1 1601311172.5475047

Client start receives
Test for: /queue/junk
Bad test: false
Test for: /queue/1601311172.5474803
Good test: false
Received: message payload: 1 1601311172.5475047

Client close complete

Why ?

Also: this method needs to be available to users of Stomp#Connection, not just Clients.

@sudeeptarlekar
Copy link
Author

sudeeptarlekar commented Oct 15, 2020

@gmallard I have added method to check subscriber for Stomp#Connection as well, please have a look.

In example that you shared while subscribing to queue, client is using header with uuid. We need to pass same header set to subscribed? method while checking subscriber, as shown in sample code below

bqn = '/queue/junk'
puts "Test for: #{bqn}"
puts "Bad test: #{client.subscribed?(bqn)}" # returns false
puts "Test for: #{qname}"
puts "Good test: #{client.subscribed?(qname, { 'id' => uuid })}" # returns true

Let me know if I am missing or doing something wrong here.

PS: Changed method name to subscribed? from subscriber? as it increases readability.

@sudeeptarlekar
Copy link
Author

@gmallard did you got chance to look at this?

@gmallard
Copy link

gmallard commented Oct 28, 2020 via email

@gmallard
Copy link

Well I totally lost a previous comment.

Here it is again. Let's try a different approach to getting this done.

What I want from this modification:

For Stomp::Client:

From clie1.rb -

    d = "/queue/abc"
    client.subscribe(d, {}) {|msg|
      p [ "msg", msg ]
    }
    #
    puts "subscribed? results for clie1: #{client.protocol()} #{client.subscribed?(d)}"

Wanted Results:
From Protocol 1.0: subscribed? results for clie1: 1.0 true
From Protocol 1.1: subscribed? results for clie1: 1.1 true
From Protocol 1.2: subscribed? results for clie1: 1.2 true

From clie2.rb -

    d = "/queue/abc2"
    sid = "my-sub-id"
    client.subscribe(d, {:id => sid}) {|msg|
      p [ "msg", msg ]
    }
    #
    puts "subscribed? results for clie2: #{client.protocol()} #{client.subscribed?(sid)}"

Wanted Results:
From Protocol 1.0: subscribed? results for clie2: 1.0 true
From Protocol 1.1: subscribed? results for clie2: 1.1 true
From Protocol 1.2: subscribed? results for clie2: 1.2 true

=====================================================================

For Stomp::Connection:

From conn1.rb -

    d = "/queue/conn1"
    conn.subscribe(d, {})
    puts "results:"
    puts conn.subscribed?(d)

Wanted Results:
From Protocol 1.0: subscribed? results for conn1: 1.0 true
From Protocol 1.1: .... connection.rb:356:in subscribe': a valid subscription id header is required (Stomp::Error::SubscriptionRequiredError) From Protocol 1.2: .... connection.rb:356:in subscribe': a valid
subscription id header is required
(Stomp::Error::SubscriptionRequiredError)

From conn2.rb -

    d = "/queue/conn2"
    sid = "asub-id"
    conn.subscribe(d, {:id => sid})
    puts "subscribed? results for conn2: #{conn.protocol()} #{conn.subscribed?(sid)}"

Wanted Results:
From Protocol 1.0: subscribed? results for conn2: 1.0 true
From Protocol 1.1: subscribed? results for conn2: 1.1 true
From Protocol 1.2: subscribed? results for conn2: 1.2 true


From conn3.rb - 
```ruby
    d = "/queue/conn3"
    sid = "asub-id"
    sid2 = "bsub-id"
    conn.subscribe(d, {:id => sid}, sid2)
    puts "subscribed? results for conn3: #{conn.protocol()} #{conn.subscribed?(sid2)}"

Wanted Results:
From Protocol 1.0: subscribed? results for conn3: 1.0 true
From Protocol 1.1: subscribed? results for conn3: 1.1 true
From Protocol 1.2: subscribed? results for conn3: 1.2 true

================================================================================

What I am currently getting with your PR:

$ ./runall.sh
proto 1.0
clie1 ...
Client Connect complete
subscribed? results for clie1: 1.0 true

Client close complete
proto 1.1
clie2 ...
Client Connect complete
subscribed? results for clie2: 1.1 false

Client close complete

proto 1.0
conn1 ...
Connection complete.
subscribed? results for conn1: 1.0 true

Connection disconnect complete
proto 1.1
conn2 ...
Connection complete.
subscribed? results for conn2: 1.1 false

Connection disconnect complete
proto 1.2
conn3 ...
Connection complete.
subscribed? results for conn3: 1.2 true

Connection disconnect complete

So .... give this all another shot please.

@sudeeptarlekar
Copy link
Author

sudeeptarlekar commented Nov 8, 2020

For client

I am not getting what am I missing here, but implementation for subscribed? methods clearly asking for destination and then headers.

def subscribed?(destination, headers = {})

I see in clie2.rb you are passing subscriber ID instead of the destination, which should be

 puts "subscribed? results for clie2: #{client.protocol()} #{client.subscribed?(d, {:id => sid })}"  # => returns true for all protocols

For connection

In order to maintain the subscription reference connection has to be reliable and for protocol >= 1.1 I am getting error from this line

for conn2.rb we need to pass the destination and not the subscriber ID. Same goes for conn3.rb.

We should passing destination and subscriber id for new subscribed? method

for client.rb

client.subscribed?(destination, { :id => subscriber_id })

for connection.rb

connection.subscribed?(destination, subscriber_id)

@gmallard
Copy link

Well, we are getting nowhere fast here .........

Sorry about the mangled code above.

Just one parameter to to 'subscribed?' - you are correct.

Regarding dest vs sid - you should have to use what you supplied on the 'subscribe'.

Calls to subscribe and unsubscrbe should behave exactly as they do now. 1.1 is a bit different here. History.

Lets get some code out please and let me take a look at it.

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.

Is there any method to check if subscription is present?
2 participants