feat: context cancellation #5
Conversation
| if err != nil { | ||
| if cancelErr != nil { | ||
| return fmt.Errorf("upload canceled: %v, but failed to cancel upload session: %v", ctx.Err(), cancelErr) | ||
| } | ||
| return err | ||
| } |
There was a problem hiding this comment.
We should still log something if the session fails to cancel but no error in the download (cancelErr != nil but err == nil). Also, this should be download, not upload.
There was a problem hiding this comment.
It is a weird use case where the cancellation will not work so the current download will continue...
I didn't want to add logs inside the Go wrapper in order to let the application consuming it make the choice, but I agree we need to handle this issue.
There was a problem hiding this comment.
Should we return an error ?
There was a problem hiding this comment.
Should we return an error ?
Yes, I think that's a great idea.
There was a problem hiding this comment.
I think we're still missing the condition (cancelErr != nil && err == nil)...?
There was a problem hiding this comment.
The function return now cancelErr so if it is not nil (meaning that an error occurred during the cancellation) it will be returned.
| cancelErr = node.DownloadCancel(cid) | ||
| default: | ||
| // continue | ||
| } |
There was a problem hiding this comment.
The way I understand this is that the context cancellation is checked only once and is non-blocking due to the default, after download is started. Would it be better to start a go routine and let it cancel the session so that context cancellation can be monitored continuously?
var cancelErr error
go func() {
<-ctx.Done()
cancelErr = node.DownloadCancel(cid)
}()
_, err = bridge.wait()
if err != nil {
// ...I have one question though. If we call C.cGoCodexDownloadStream, then the context is cancelled and node.DownloadCancel is called, will the resources consumed in C.cGoCodexDownloadStream be cleaned up properly? I also find the ergonomics of the two calls to be oddly different:
C.cGoCodexDownloadStream
node.DownloadCancelBut both seem to go through the thread boundary back to nim. Or maybe I misunderstood something here?
There was a problem hiding this comment.
The way I understand this is that the context cancellation is checked only once and is non-blocking due to the default, after download is started. Would it be better to start a go routine and let it cancel the session so that context cancellation can be monitored continuously?
Oh yes you're right, thanks !
will the resources consumed in C.cGoCodexDownloadStream be cleaned up properly?
Yes it will. If there is a cancellation, the stream will be closed, the current call to cGoCodexDownloadStream will generate an error. The bridge.wait() will wait until the error is sent and the resources will be freed thanks to defer bridge.free().
There was a problem hiding this comment.
Out of curiosity, does this pattern not work?
var cancelErr error
go func() {
<-ctx.Done()
cancelErr = node.DownloadCancel(cid)
}()
if cancelErr != nil:
// ...There was a problem hiding this comment.
Sorry I didn't reply directly to your pattern, but the problem is that the goroutine will leak because it will wait infinitely for cancellation.
We agreed on discord on using defer close(done) to close the goroutine after the function is done.
| select { | ||
| case <-ctx.Done(): | ||
| cancelErr = node.UploadCancel(sessionId) | ||
| default: | ||
| // continue | ||
| } |
There was a problem hiding this comment.
Same comment as above re: go routine.
| } | ||
|
|
||
| return bridge.wait() | ||
| done := make(chan struct{}) |
There was a problem hiding this comment.
Should we also add defer node.UploadCancel on line 300 to keep with the pattern?
There was a problem hiding this comment.
The code has changed now, the same pattern as DownloadStream has been applied.
There was a problem hiding this comment.
Oh sorry I was lost in the different comments / changes. You meant to add the "preventive" defer cancel. Yep will add it.
…celled after the function is done
…ause it is a sync call
2 participants
This PR add context and cancellation for
UploadReader,UploadFileandDownloadStream.