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

Would be good to use http default client #5

Open
rusenask opened this issue Aug 24, 2024 · 4 comments · May be fixed by #7
Open

Would be good to use http default client #5

rusenask opened this issue Aug 24, 2024 · 4 comments · May be fixed by #7

Comments

@rusenask
Copy link

This (https://github.com/mendableai/firecrawl-go/blob/main/firecrawl.go#L197-L199)

	client := &http.Client{
		Timeout: 60 * time.Second,
	}

Doesn't respect proxy vals and creates a new keepalive goroutine whenever a new firecrawl client is initialized

@rafaelsideguide
Copy link
Collaborator

@rusenask "Go’s http package doesn’t specify request timeouts by default, allowing services to hijack your goroutines" (https://medium.com/@nate510/don-t-use-go-s-default-http-client-4804cb19f779)

@rusenask
Copy link
Author

yeah but you should still just then get the transport from the http default client. Another option is:

func (app *FirecrawlApp) makeRequest(method, url string, data map[string]any, headers map[string]string, action string, opts ...requestOption) ([]byte, error) {
	var body []byte
	var err error
	if data != nil {
		body, err = json.Marshal(data)
		if err != nil {
			return nil, err
		}
	}

	req, err := http.NewRequest(method, url, bytes.NewBuffer(body))
	if err != nil {
		return nil, err
	}

Use the context here:

func (app *FirecrawlApp) makeRequest(ctx context.Context,  method, url string, data map[string]any, headers map[string]string, action string, opts ...requestOption) ([]byte, error) {
	var body []byte
	var err error
	if data != nil {
		body, err = json.Marshal(data)
		if err != nil {
			return nil, err
		}
	}
        ctx, cancel := context.WithTimeout(ctx, 60 * time.Second)
       defer cancel()
	req, err := http.NewRequesWithContextt(ctx, method, url, bytes.NewBuffer(body))
	if err != nil {
		return nil, err
	}

This way you can both cancel from the caller's side and also have the timeout within the function without modifying the transport. Especially since you already have the helper makeRequest request function

@KentHsu
Copy link
Contributor

KentHsu commented Aug 29, 2024

Isn't current setting using the default transport? Do we need to make it explicitly using the default transport?

client := &http.Client{
    Timeout: 60 * time.Second,
    Transport: http.DefaultTransport,
}

@rusenask
Copy link
Author

Isn't current setting using the default transport? Do we need to make it explicitly using the default transport?

client := &http.Client{
    Timeout: 60 * time.Second,
    Transport: http.DefaultTransport,
}

yup, if you not set it then you will get a clean one without things like inheriting proxy configuration and keep-alive goorutine

@KentHsu KentHsu linked a pull request Sep 8, 2024 that will close this issue
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 a pull request may close this issue.

3 participants