Skip to content

Conversation

@YmBIgo
Copy link

@YmBIgo YmBIgo commented Aug 10, 2025

Thanks for this great project.

While exploring codebase, I found some suspicious behaviors related to err handling and resource closing , therefore I opened PR.


First issue related to err handling @ runConnectionWebSocketAgentCommand & runConnectionWebrtcAgentCommand

I think code below do not exit when adb connect is failed.
Should we return err when adb connect failed? or is this intended ?

	if err := opts.ADBServerProxy.Connect(ret.Status.ADB.Port); err != nil {
		c.PrintErrf("Failed to connect ADB to device %q: %v\n", device, err)
	}

Second issue related to resource closing @ runConnectionWebSocketAgentCommand

I think code below do not close the net.Listen resource.
Should we add defer l.Close() ?

	l, err := net.Listen("tcp", "127.0.0.1:0")
	if err != nil {
		c.PrintErrf("Failed to listen ADB socket: %v", err)
		return err
	}

Thank you for taking time for reading my small fix.

@google-cla
Copy link

google-cla bot commented Aug 10, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link
Member

@jemoreira jemoreira left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

The return err changes do not apply, but the defer l.Close() does. In addition to fixing the PR you need to sign the Google CLA before we can merge your change, follow the instructions from the google-cla bot.

// Ask ADB server to connect even if the connection to the device already exists.
if err := opts.ADBServerProxy.Connect(ret.Status.ADB.Port); err != nil {
c.PrintErrf("Failed to connect ADB to device %q: %v\n", device, err)
return err
Copy link
Member

Choose a reason for hiding this comment

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

The main purpose of the function is to keep a port open that exposes the device's adb daemon in this host. This particular code instructs the adb server in this host to connect to that port. That may fail for several reasons, the most common one being no adb server is currently running; but none of those failures interferes with the main purpose of the function so the error is only logged.

err = opts.ADBServerProxy.Connect(adbPort)
if err != nil {
c.PrintErrf("Failed to connect ADB to device: %v\n", err)
return err
Copy link
Member

Choose a reason for hiding this comment

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

same as before, this is not fatal.

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