BROKEN: Duplicate messages on python client (and elsewhere)

[mikael-s-persson]

The python client test is broken right now. I bisected the issue down to one commit.

This commit:
5435370

Breaks the python client test:

$ bazel test --test_output=streamed -c dbg --config=libc++ //client/python:client_test 
WARNING: Streamed test output requested. All tests will be run locally, without sharding, one at a time
INFO: Analyzed target //client/python:client_test (0 packages loaded, 4 targets configured).
F.
======================================================================
FAIL: test_one_message (__main__.TestSubspaceClient.test_one_message)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/bazel-working-directory/_main/bazel-out/k8-dbg/bin/client/python/client_test.runfiles/_main/client/python/client_test.py", line 69, in test_one_message
    self.assertEqual(received_message, "Hello again!")
AssertionError: 'Hello world!' != 'Hello again!'
- Hello world!
+ Hello again!


----------------------------------------------------------------------
Ran 2 tests in 0.014s

FAILED (failures=1)
FAIL: //client/python:client_test

I'm seeing additional breakages on another set of tests that I have in our code-base. But they seem similar to this python failure, getting lots of duplicated messages.

[dallison]

Sorry about that, Mik. If you revert the commit does it fix the python client? I don't have any way to run the python stuff unfortunately. It's probably that subscribers are not being deleted by the python code. I will try to run, but I don't have any access to Linux until Sunday evening. Any help appreciated. The commit was something I have also done in work but we aren't seeing duplicates. I'll dig into it.

[dallison]

Are all the other things that are failing using the python client too? Or is this broken in C++?

[mikael-s-persson]

No the other things are in C++.

[dallison]

Duplicates in C++? I'm not seeing that in my tests. Can you tell me how many subscribers you have and how you are reading the messages? Are you using subspace::shared_ptrs? Or just draining the channel when the subscriber is triggered?

[dallison]

I'd love to write a test to repro, do you have one?

[mikael-s-persson]

It's just draining the messages after triggering. We are not using the shared-ptr stuff (I can't recall, but something about that was unusable).

[dallison]

I can repro it in python on my mac. On it.

[mikael-s-persson]

If it helps, the symptom I see is that after each trigger on a subscriber, ReadMessage(Next) seems to essentially reset to the beginning of history of the channel, as if it completely forgot what slots had already been read. For example, one of my test is pretty simple pub-sub message by message and checking that everything published is what is received, but what I received is something like: ["Msg1", "Msg1", "Msg2", "Msg1", "Msg2", "Msg3", ...]... so the subscriber wakes up on "Msg1" and reads "Msg1", then wakes up on "Msg2" but reads both "Msg1" and "Msg2".

[dallison]

Found the issue. It was clearing the current slot in the subscriber after it ran out of messages. That's wrong.

[dallison]

Alright, fix has been pushed. Can you try it and make sure it fixes your issue please. Sorry about that. I also added a regression test for the C++ client.

[dallison]

BTW, I am open to a google meet if you would like to report these to get a quicker turnaround.

[dallison]

We are using shared_ptrs in Cruise and they are working fine for us. If you can remember why they weren't any use maybe I can make them more useful.

[mikael-s-persson]

Yep! That fixes the issue on our tests as well.

[dallison]

Cool, I'll do another release.

[mikael-s-persson]

If you can remember why they weren't any use maybe I can make them more useful.

Oh yeah, I remember now. We use the timestamp and ordinal from the Message returned from ReadMessage, for logging, ordering and detecting losses. So, in effect, what would be useful is a subspace::shared_ptr<subspace::Message> that doesn't cast, but just keeps the ref-count.