Skip to content

Bugfixes#13

Open
AbdullahGheith wants to merge 12 commits intoft-t:masterfrom
AbdullahGheith:master
Open

Bugfixes#13
AbdullahGheith wants to merge 12 commits intoft-t:masterfrom
AbdullahGheith:master

Conversation

@AbdullahGheith
Copy link
Copy Markdown

Fixed:

  • When clicking "Save All", dont do the request
  • Don't do the request the request every 30 second if Interval is not set
  • If browser URL is not filled, do the API request without opening the browser.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces several improvements, including the addition of an 'InsecureSkipVerify' option for HTTP requests, enhanced panic recovery across event handlers, and a more robust ticker-based execution loop for background tasks. Regarding the review feedback, it is recommended to optimize the HTTP client management in 'pkg/executor/executor.go' by reusing clients instead of creating a new one for every request to prevent potential socket exhaustion.

Comment thread pkg/executor/executor.go
Comment on lines +32 to +36
client := req.C()
if executeReq.Config.InsecureSkipVerify {
client.SetTLSClientConfig(&tls.Config{InsecureSkipVerify: true})
}
httpReq := client.NewRequest()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Creating a new HTTP client with req.C() for every Execute call is inefficient. http.Client (which req.Client wraps) is designed for reuse. Frequent creation can lead to resource issues like socket exhaustion (TIME_WAIT state).

To improve this, you could create and manage req.Client instances within the Executor struct. For example, you could have one client for secure connections and another for insecure connections, and choose the appropriate one here based on executeReq.Config.InsecureSkipVerify.

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.

1 participant