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

opt: delete redundant channel connection.incomingCmdCh #1343

Merged
merged 3 commits into from
Mar 7, 2025

Conversation

gunli
Copy link
Contributor

@gunli gunli commented Mar 7, 2025

Fixes #

Master Issue: #1335 #1336

Motivation

Reading is run in a seperate goroutine, connection.incomingCmdCh is redundant, just call connection.internalReceivedCommand() directly.

func (c *connection) run() {
	pingSendTicker := time.NewTicker(c.keepAliveInterval)
	pingCheckTicker := time.NewTicker(c.keepAliveInterval)

	defer func() {
		// stop tickers
		pingSendTicker.Stop()
		pingCheckTicker.Stop()

		// all the accesses to the pendingReqs should be happened in this run loop thread,
		// including the final cleanup, to avoid the issue
		// https://github.com/apache/pulsar-client-go/issues/239
		c.failPendingRequests(errConnectionClosed)
		c.Close()
	}()

	// All reads come from the reader goroutine
	go c.reader.readFromConnection()  // reading is run in a SEPERATE goroutine here
	go c.runPingCheck(pingCheckTicker)
}

Modifications

delete redundant channel connection.incomingCmdCh

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: (yes / no)
  • The schema: (yes / no / don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable / docs / GoDocs / not documented)
  • 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

@gunli gunli mentioned this pull request Mar 7, 2025
1 task
@gunli
Copy link
Contributor Author

gunli commented Mar 7, 2025

Copy link
Member

@nodece nodece left a comment

Choose a reason for hiding this comment

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

LGTM

@nodece
Copy link
Member

nodece commented Mar 7, 2025

In the Golang, we must use the separate goroutine to perform the read and write operation.

@BewareMyPower BewareMyPower added this to the v0.15.0 milestone Mar 7, 2025
Copy link
Member

@crossoverJie crossoverJie left a comment

Choose a reason for hiding this comment

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

LGTM

@nodece nodece merged commit 6cb2867 into apache:master Mar 7, 2025
7 checks passed
gunli added a commit to gunli/pulsar-client-go that referenced this pull request Mar 7, 2025
crossoverJie pushed a commit that referenced this pull request Mar 7, 2025
* rebase to master to merge #1343

* make consumer is not nil

* revert test case

* revert test case
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.

4 participants