tursodatabase / libsql-client-go

Go client API for libSQL
MIT License
180 stars 24 forks source link

Minor improvements #9

Closed avinassh closed 1 year ago

avinassh commented 1 year ago
  1. Pass ctx to HTTP calls.

I noticed the library isn't passing the context down to internal methods. This is important since we use ctx for handling deadlines and timeout.

For the user, if they want to cancel the operation if it isn't completed in under 1 second, the code would be:

ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
defer cancel()
res, err := db.QueryContext(ctx, stmt, args...)

to fix this, we must pass the ctx to the callSqld method and use that in the http calls.

  1. Add timeout to HTTP calls

The library currently uses the default http package from the stdlib. The calls from this package don't have any timeouts set, so if the server isn't responding, libsql client may get stuck indefinitely. Usually, it is better to set some timeout on http client itself (which will apply to all the calls).

Adding a timeout solves this:

// the http call has to finish in under 60 seconds
// a global variable which all of http package code will use
var HTTPClient = &http.Client{Timeout: 60 * time.Second}

Another alternative is to use hashicorp's cleanhttp package which comes with some sane defaults


I am happy to send a PR to fix these!

penberg commented 1 year ago

@avinassh sorry, missed this, please do send a pull request!

haaawk commented 1 year ago

I'm actually working on this change so please don't listen to @penberg :)

avinassh commented 1 year ago

Oh, Let me find something else to contribute!

haaawk commented 1 year ago

Actually, I won't have time to wrap up this work in the next few days so I think it's best I leave it for you @avinassh :)

avinassh commented 1 year ago

I have opened a PR #15 which passes the ctx to http calls. This fixes the first part of the issue.

For the second one, suggest me a safe default timeout to set? How does 120s sound?

haaawk commented 1 year ago

Thanks @avinassh. Could you do the same fix for websockets please?

avinassh commented 1 year ago

I opened a PR #16 to add changes for websockets and also added timeout for http driver.

Go sql driver now uses ctx for most methods and methods which don't take ctx are deprecated. I believe that should be done here too.

haaawk commented 1 year ago

Thanks @avinassh. I think we can close this issue now.