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

[C++] Add test exhibiting recording descriptor bug in stored channel strings #1348

Conversation

jrsala-auguration
Copy link
Contributor

This is not a finished PR. This just adds a failing test that should pass.

The EXPECT_EQ(originalChannel, m_recordingChannel); assertion fails as follows:

AeronArchiveTest.cpp:1402: Failure
Expected equality of these values:
  originalChannel
    Which is: "aeron:udp?endpoint=localhost:3333\t"
  m_recordingChannel
    Which is: "aeron:udp?endpoint=localhost:3333"

There's always an extra byte at the end of the channel strings in recording descriptors, whether it's originalChannel or strippedChannel or sourceIdentity.

I don't know what the solution is. The generated SBE code seems correct. For good measure I added that same assertion to the jumbo recording descriptors test, which fails in the same way.

@jrsala-auguration
Copy link
Contributor Author

Note that the CI checks for the various GCC and Clang versions succeed despite the test failing. The Windows check fails as it should. Maybe something is not right in the CI setup if test outcomes are ignored.

@tmontgomery
Copy link
Contributor

My guess on this is that the RecordingDescriptorPoller is running off the end of the string due to not using the string version of the get field. i.e. getOriginalChannelAsString. The compiler might be making an explicit conversion from a const char * to a std::string which doesn't have a length, so the end of the string has the next field varDataEncoding.

@jrsala-auguration
Copy link
Contributor Author

@tmontgomery Things I also tried that didn't work in ControlResponseAdapter.cpp, where m_onRecordingDescriptor is called:

descriptor.getOriginalChannelAsString(),
std::string(descriptor.originalChannel(), descriptor.originalChannelLength()),

I looked at the encoding side in Java class RecordingDescriptorEncoder and it seemed fine. It seems to be used correctly in Catalog#addNewRecording.

@tmontgomery
Copy link
Contributor

No. It is actually as I mentioned. I've added your tests and my fix and seems fine. Will push once I run a cppbuild.

tmontgomery added a commit that referenced this pull request Aug 17, 2022
@tmontgomery
Copy link
Contributor

commit is d95157f

@jrsala-auguration
Copy link
Contributor Author

fixed

@jrsala-auguration jrsala-auguration deleted the recording-descriptor-bad-channel-strings branch August 18, 2022 09:40
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.

2 participants