Skip to content

Conversation

astuyve
Copy link
Contributor

@astuyve astuyve commented Aug 20, 2025

…riptor leakage

What does this PR do?

Reads the full response and closes the connection to prevent the server from holding the connection open

Motivation

Testing Guidelines

Additional Notes

Types of changes

  • Bug fix
  • New feature
  • Breaking change
  • Misc (docs, refactoring, dependency upgrade, etc.)

Checklist

  • This PR's description is comprehensive
  • This PR contains breaking changes that are documented in the description
  • This PR introduces new APIs or parameters that are documented and unlikely to change in the foreseeable future
  • This PR impacts documentation, and it has been updated (or a ticket has been logged)
  • This PR's changes are covered by the automated tests
  • This PR collects user input/sensitive content into Datadog

@astuyve astuyve requested a review from a team as a code owner August 20, 2025 17:59
@@ -14,6 +14,7 @@ import (
"encoding/base64"
"encoding/json"
"fmt"
"io"
"net/http"
Copy link
Contributor

Choose a reason for hiding this comment

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

There are 4 places in this file where we call Client.Do. We should be sure to fix all of them. Esp start/end invocation and flush.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flush is a local test thing only, I don't actually think it works anymore

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not what I'm seeing at

We call Flush every time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably the better thing to do is what you are saying, ensure the Flush call only happens when local testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the tracer flush, not calling the flush endpoint on the extension

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh duh, thank you

@astuyve astuyve merged commit c0b16b2 into main Aug 21, 2025
9 checks passed
@astuyve astuyve deleted the aj/read-response-close-connection branch August 21, 2025 01:15
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