mendableai / firecrawl-go

MIT License
2 stars 1 forks source link

Would be good to use http default client #5

Open rusenask opened 3 weeks ago

rusenask commented 3 weeks ago

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 commented 2 weeks ago

@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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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