Skip to content

Conversation

@estolfo
Copy link
Contributor

@estolfo estolfo commented Oct 27, 2025

This PR adds a method that checks to make sure channels are created before they are published to in the tests.

The tests were hanging because there was a race condition:

  • The tests sometimes attempted to publish an event before the subscribe request was complete and a channel was created
  • The first event therefore wasn't enqueued
  • Then, the test tried to pop events from the queue and the first event popped was actually the second event ('c2' or 'p2')
  • Then the test tried to pop a second event and would block waiting indefinitely, but there was no second event, as the first was never enqueued.

The new method checks in a loop with a timeout that a channel exists so that events can be successfully published.

Here are some print statements from debugging that show that the first event popped from the queue is actually the second event:

w Enable Watchlogstash-1  | channel: starting run thread with queue: 4096, queue.size: 0
w Enable Watchlogstash-1  | starting publish thread queue: 4096, queue.size: 0
w Enable Watchlogstash-1  | popping first event
w Enable Watchlogstash-1  | popped message {"c":"2"}
w Enable Watchlogstash-1  | popping second event

Additionally, I found that if the event was attempted to be published and enqueued before the subscription had completed, the redis client would return a 0 from the publish method call. It should return 1 if the event was successfully published.

sending event c1
@client.call command [:publish, "foo", "{\"data\":\"c1\"}"]: #<Method: Redis::Client#call(command) .../.rvm/gems/jruby-9.4.13.0/gems/redis-4.8.1/lib/redis/client.rb:160>
reply: 0
sending c2
@client.call command [:publish, "foo", "{\"data\":\"c2\"}"]: #<Method: Redis::Client#call(command) .../.rvm/gems/jruby-9.4.13.0/gems/redis-4.8.1/lib/redis/client.rb:160>
reply: 1

I've also updated the data that is published to be a json object, as there were intermittent errors:

[2025-10-28T08:54:44,076][ERROR][logstash.codecs.json     ] JSON parse error, original data now in message field {:message=>"Unrecognized token 'pc1': was expecting (JSON String, Number, Array, Object or token 'null', 'true' or 'false')\n at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 1, column: 4]", :exception=>LogStash::Json::ParserError, :data=>"pc1"}

Resolves #97

@jsvd jsvd self-requested a review October 28, 2025 15:52
Copy link
Member

@jsvd jsvd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nitpick, otherwise LGTM, thanks for not conceding to sleep :D

Copy link
Member

@jsvd jsvd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we leverage a let here and simply use try { expect() } it would greatly reduce the amount of code needed for this change

Copy link
Member

@jsvd jsvd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@estolfo estolfo merged commit e8bd6e5 into logstash-plugins:main Oct 31, 2025
3 checks passed
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.

Integration tests for data types "channel" and "pattern channel" occasionally hang

2 participants