Skip to content

Comments

Ensure proper closure of HTTP response body to prevent resource leak#331

Open
SahibYar wants to merge 7 commits intoalpacahq:masterfrom
SahibYar:resource-leak-fix
Open

Ensure proper closure of HTTP response body to prevent resource leak#331
SahibYar wants to merge 7 commits intoalpacahq:masterfrom
SahibYar:resource-leak-fix

Conversation

@SahibYar
Copy link
Contributor

This PR fixes a potential resource leak issue in code. This oversight could lead to resource exhaustion over time. (Reference)

Changes:

  • Explicitly added resp.Body.Close() in all relevant places where HTTP requests are made.
  • Ensures that resources are properly released after the response is handled.
  • Added few nil check to avoid nil potential dereference panics.

Testing.

I executed all test cases in following files.

marketdata/rest_test.go
marketdata/options_test.go

alpaca/rest_test.go
alpaca/stream_test.go
alpaca/trading_test.go

@SahibYar SahibYar force-pushed the resource-leak-fix branch from ac09453 to 644a290 Compare March 17, 2025 18:03
@SahibYar SahibYar force-pushed the resource-leak-fix branch from 644a290 to 28dad51 Compare March 17, 2025 18:06
}

func verify(resp *http.Response) error {
func Verify(resp *http.Response) error {
Copy link
Collaborator

@gnvk gnvk Mar 18, 2025

Choose a reason for hiding this comment

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

Please do not make this and closeResp exported. I understand the intent (to reuse them in the marketdata package), but that isn't worth making these public. On the long run we can consider introducing an internal package and refactor the common code between alpaca and marketdata, but for now let's just keep the duplication please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if I don't make Verify exported, I have to copy all the following chunk of code copied to marketdata just to keep the logic same.

func verify(resp *http.Response) error {
	if resp.StatusCode >= http.StatusMultipleChoices {
		return APIErrorFromResponse(resp)
	}
	return nil
}
func APIErrorFromResponse(resp *http.Response) error {
	defer closeResp(resp)

	body, err := io.ReadAll(resp.Body)
	if err != nil {
		return err
	}
	var apiErr APIError
	if err := json.Unmarshal(body, &apiErr); err != nil {
		// If the error is not in our JSON format, we simply return the HTTP response
		return fmt.Errorf("%s (HTTP %d)", body, resp.StatusCode)
	}
	apiErr.StatusCode = resp.StatusCode
	apiErr.Body = strings.TrimSpace(string(body))
	return &apiErr
}
func closeResp(resp *http.Response) {
	if resp == nil || resp.Body == nil {
		return
	}
	// The underlying TCP connection cannot be reused if the body is not fully read
	_, _ = io.Copy(io.Discard, resp.Body)
	resp.Body.Close()
}
type APIError struct {
	StatusCode int    `json:"-"`
	Code       int    `json:"code"`
	Message    string `json:"message"`
	Body       string `json:"-"`
}
func (e *APIError) Error() string {
	if e.Code != 0 {
		return fmt.Sprintf("%s (HTTP %d, Code %d)", e.Message, e.StatusCode, e.Code)
	}
	return fmt.Sprintf("%s (HTTP %d)", e.Message, e.StatusCode)
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's fine.

Copy link
Contributor Author

@SahibYar SahibYar Mar 20, 2025

Choose a reason for hiding this comment

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

actually I am raising another PR, which will remove much of redundant code. #334 It is not rigorous code change, targeting 1 file at a time. Ultimately we can create a common module for redundant code among marketdata and alpaca and move common functions like Verify and CloseResp to a separate make named alpaca-core

and we can call those functions like this

alpacaCore.Verfiy()
alpacaCore.CloseResp()

Copy link
Collaborator

Choose a reason for hiding this comment

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

alpacaCore is definitely not a good package name and I'm a bit hesitant about this. Could you please focus on finishing this PR (keeping the code duplication between alpaca and marketdata) before doing more refactors?

@gnvk gnvk changed the title Fix: Ensure proper closure of HTTP response body to prevent resource leak Ensure proper closure of HTTP response body to prevent resource leak Mar 18, 2025
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