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(plugin test): close connection in plugin test environment #211

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zryanl
Copy link

@zryanl zryanl commented Aug 19, 2024

  • Addresses a connection leak in the internal plugin test environment's network connection instantiation
  • Implements some extra error handling and checks in the plugin test package

@CLAassistant
Copy link

CLAassistant commented Aug 19, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

- Addresses a connection leak in the internal plugin test environment's network connection instantiation
@@ -20,7 +20,7 @@ func readPbFrame(conn net.Conn) (data []byte, err error) {
var len uint32
err = binary.Read(conn, binary.LittleEndian, &len)
if err != nil {
return
return nil, err
Copy link
Member

Choose a reason for hiding this comment

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

Note that these and others below make use of named return values, so these changes have no effect.

Copy link
Member

@gszr gszr left a comment

Choose a reason for hiding this comment

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

@zryanl Thanks for your contribution; please restrict this PR to only the close connection fix and I will be happy to re-review.

@@ -122,19 +122,23 @@ func (req *Request) Validate() error {
}
return nil
}
return fmt.Errorf("Unsupported method \"%v\"", req.Method)
Copy link
Member

Choose a reason for hiding this comment

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

Let's not change this error message.

}
portnum, _ := strconv.Atoi(p)
return int32(portnum)
portnum, err := strconv.ParseInt(p, 10, 32)
Copy link
Member

Choose a reason for hiding this comment

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

The port is already validated in the URL object creation, so this should be unreachable.

}

// New creates a new test environment.
func New(t *testing.T, req Request) (env *TestEnv, err error) {
err = req.Validate()
if err != nil {
return
return nil, err
Copy link
Member

Choose a reason for hiding this comment

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

Named return value. Let's not change this and others.

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.

3 participants