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

AcknowledgeExtension NPE #810

Open
supersteves opened this Issue Dec 18, 2018 · 7 comments

Comments

Projects
None yet
2 participants
@supersteves
Copy link

supersteves commented Dec 18, 2018

We've upgraded CometD / Jetty from 3.0.4 / 9.2 to 3.1.5 / 9.4.

We use vanilla CometD except we add in the Ack extension only.

Everything's great in real-world use (JS websocket client, Java server). However some of our tests use entirely server-side LocalSessions to simulate clients and assert high level message sequences in our app.

When running these tests locally, we get NPE here because batch is null Long and being passed to a long arg method:

(I notice that in 4.0 it no longer depends on thread IDs. However I'm not sure I'm ready for 4.0 since it's only been out of beta since September.)

In debugging I've observed that sendMeta is called for META_CONNECT (populating _batches) but then deQueue is called twice. The 2nd time _batches is empty.

@sbordet

This comment has been minimized.

Copy link
Member

sbordet commented Dec 18, 2018

LocalSession does not heartbeat, so it does not make sense to attach an ack extension to it.
Do you have a reproducible test case I can look at for the exact details?

@supersteves

This comment has been minimized.

Copy link

supersteves commented Dec 18, 2018

This is why I was reticent to file... no. It's tightly woven into closed source.

@supersteves

This comment has been minimized.

Copy link

supersteves commented Dec 18, 2018

We use ack for order guarantee. Does that not matter for LocalSession?

@sbordet

This comment has been minimized.

Copy link
Member

sbordet commented Dec 18, 2018

LocalSession is delivered messages synchronously as they are published, so the message order is implicitly guaranteed, so you don't need to add the ack extension to LocalSessions.

Don't be reticent to file issues, they are an important contribution to the project.

You can perhaps detail in words (or pseudo code) what your closed source test does?

@supersteves

This comment has been minimized.

Copy link

supersteves commented Dec 18, 2018

OK sure.

First we create 2 test "clients", each being:

  • session.newLocalSession(unique name)
  • session.addExtension(new AckExtension())
  • session.handshake()
  • session.getChannel(channelId).subscribe(this::receive, callback)
  • The callback gets called, it's successful, in this callback...
  • session.getChannel(submitChannel).publish(initialMsg, callback)
  • The callback is successful

From the server's perspective (this is production code):

  • when the server received the published message (in the test thread), it queued an event it some other thread
  • later, the other thread...
  • look up the ServerSession
  • call ServerSession.deliver(sender, channel, responseMsg)

The responseMsg never arrives in the test client's 'receive' method, because of the NPE.

I think that's it.

@sbordet

This comment has been minimized.

Copy link
Member

sbordet commented Dec 18, 2018

So yes, the ack extension won't work on LocalSession - it is based on the heartbeat which is absent in LocalSessions.

You can easily replace your clients with BayeuxClient instances, and then the ack extension will work fine.

@supersteves

This comment has been minimized.

Copy link

supersteves commented Dec 18, 2018

Thanks. I'm working on that already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment