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

Raise ArgumentError if nil passed to Consumer#subscribe #463

Closed
wants to merge 1 commit into from
Closed

Raise ArgumentError if nil passed to Consumer#subscribe #463

wants to merge 1 commit into from

Conversation

eugene-nikolaev
Copy link

@eugene-nikolaev eugene-nikolaev commented Jun 19, 2024

Hi!
I propose to add such a guard into Consumer#subscribe.
Otherwise such a call leads to a segfault in librdkafka which is confusing and hard to troubleshoot:

.../rdkafka-ruby/lib/rdkafka/consumer.rb:78: [BUG] Segmentation fault at 0x0000000000000000
ruby 3.3.3 (2024-06-12 revision f1c7b6f435) [arm64-darwin22]

-- Crash Report log information --------------------------------------------
   See Crash Report log file in one of the following locations:             
     * ~/Library/Logs/DiagnosticReports                                     
     * /Library/Logs/DiagnosticReports                                      
   for more details.                                                        
Don't forget to include the above Crash Report log file in bug reports.     

-- Control frame information -----------------------------------------------
c:0070 p:---- s:0382 e:000381 CFUNC  :rd_kafka_subscribe
c:0069 p:0008 s:0376 e:000375 BLOCK  /Users/eugene/projects/rdkafka-ruby/lib/rdkafka/consumer.rb:78
c:0068 p:0044 s:0372 e:000371 METHOD /Users/eugene/projects/rdkafka-ruby/lib/rdkafka/native_kafka.rb:77
c:0067 p:0049 s:0368 e:000367 METHOD /Users/eugene/projects/rdkafka-ruby/lib/rdkafka/consumer.rb:77
<hundreds of lines below>

That could save a lot of time of the library users.
I've spent half a day after mistakenly putting a nil there, which is very easy to do in ruby for multiple reasons :) Mine was a wrong instance variable name which was for sure undefined and dereferenced to a nil. That does not raise any error by itself.

If you're fine with that - please feel free to propose more apropriate/uniform textings or whatever.

- otherwise it leads to a segfault in librdkafka which is confusing and hard to troubleshoot
@mensfeld
Copy link
Member

I'm not sure it is within the scope of this library as:

Features that can be achieved directly in Ruby, without specific needs from librdkafka, are outside the scope of this library.

This library does not intend to offer complex producers or consumers. The aim is to stick closely to the functionalities provided by librdkafka itself.

and

Unless you are seeking specific low-level capabilities, we strongly recommend using Karafka and WaterDrop when working with Kafka. These are higher-level libraries also maintained by us based on rdkafka-ruby.

There are no ArgumentError errors and there are 13 in total of TypeError which I fundamentally do not agree with fully as well.

Adding this will justify adding more checks for consumers and producers (topic, partition, message, etc.) as each of them may cause a segfault. This is why we also maintain higher-level libraries—they have proper strict (sometimes extremely strict) validations to prevent users from making mistakes of that nature.

@mensfeld
Copy link
Member

Gave it a second thought and I stand by what I said. Closing

@mensfeld mensfeld closed this Jun 20, 2024
@mensfeld mensfeld assigned mensfeld and eugene-nikolaev and unassigned mensfeld Jun 20, 2024
@mensfeld mensfeld self-requested a review June 20, 2024 07:22
@mensfeld mensfeld added the consumer Consumer API related stuff label Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consumer Consumer API related stuff
Development

Successfully merging this pull request may close these issues.

2 participants