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

[fix][issue 1098] check batchBuilder in case batch is disabled #1099

Merged
merged 2 commits into from
Nov 1, 2023

Conversation

zzzming
Copy link
Contributor

@zzzming zzzming commented Sep 28, 2023

Fixes #1098

Motivation

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x40 pc=0x8a8eaa]

goroutine 1 [running]:

github.com/apache/pulsar-client-go/pulsar.(*partitionProducer).internalFlushCurrentBatch(0xc00c6ccc60)
/go/pkg/mod/github.com/apache/[email protected]/pulsar/producer_partition.go:869 +0x2a
github.com/apache/pulsar-client-go/pulsar.(*partitionProducer).runEventsLoop(0xc00c6ccc60)
/go/pkg/mod/github.com/apache/[email protected]/pulsar/producer_partition.go:457 +0x1d7
created by github.com/apache/pulsar-client-go/pulsar.newPartitionProducer in goroutine 5713
/go/pkg/mod/github.com/apache/[email protected]/pulsar/producer_partition.go:203 +0x9bb
when the producer option set to DisableBatching: true, the time.ticker wil be created and close after a while, when ticker.Stop() over 10ms (default value) since created it will cause the ticker still tick, and cause nil pointer dereference eventually.

Modifications

Add one more check to evaluate if batchBuilder is nil, in non-batch scenario, it should return immediate.

To understand the behaviour of ticker, write a small test code of ticker that ticker.C (the timer) sometime is called while the Stop() is called. That's why it's good to check the batchBuilder. If the batchBuilder object fails to be created, it should be caught at line 294 at producer_partition.go. So if batch is enabled, it should not be nil. Therefore we can use the nil check to stop any non-batch calls to proceed with the flush.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): ( no)
  • The public API: ( no)
  • The schema: (no )
  • The default values of configurations: (no)
  • The wire protocol: (no)

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

Copy link

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

@Gleiphir2769 Gleiphir2769 left a comment

Choose a reason for hiding this comment

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

+1

@nicoloboschi nicoloboschi merged commit 0845e73 into apache:master Nov 1, 2023
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.

[BUG] Panic when using 'DisableBatching: true' as producer option
4 participants