GODRIVER-3322 Apply client-level timeout to UploadFromStreamWithID #1782
Conversation
mongodb-drivers-pr-bot
bot
added
the
review-priority-low
API Change ReportNo changes found! |
prestonvasquez
added
review-priority-urgent
prestonvasquez
changed the title
prestonvasquez
changed the title
| }) | ||
|
|
||
| // Create a new MongoClient with timeoutMS=10. | ||
| cliOptions := options.Client().SetTimeout(10).ApplyURI(mtest.ClusterURI()) |
There was a problem hiding this comment.
Did you mean
.SetTimeout(10 * time.Millisecond)?
|
|
||
| // If the operation-level context is not respected, then the client-level | ||
| // timeout will exceed deadline. | ||
| ctx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond) |
There was a problem hiding this comment.
Can we use:
.SetTimeout(10 * time.Millisecond)?
There was a problem hiding this comment.
The goal of this test is to ensure that an operation-level context is actually used. Otherwise, this test would fail.
| }, | ||
| }) | ||
|
|
||
| // Create a new MongoClient with timeoutMS=10. |
There was a problem hiding this comment.
It will be better to update the comment to describe the code more accurately.
There was a problem hiding this comment.
Do you have something in mind? This comment is taken from the prose test: https://github.com/mongodb/specifications/tree/master/source/client-side-operations-timeout/tests#6-gridfs---upload
There was a problem hiding this comment.
Never mind, it looks good now.
There was a problem hiding this comment.
I may have missed something, but is the "timeoutMS=10" still accurate after bf50559? Where is the timeout set?
prestonvasquez
dismissed stale reviews from qingyang-hu and matthewdale
via
ee199ea
| }, | ||
| }) | ||
|
|
||
| // Create a new MongoClient with timeoutMS=10. |
There was a problem hiding this comment.
I may have missed something, but is the "timeoutMS=10" still accurate after bf50559? Where is the timeout set?
3 participants
GODRIVER-3322
Summary
bucket.UploadFromStreamandbucket.UploadFromStreamWithIDwill pre-maturely cancel a context if a client-level timeout is set since the latter method creates the context and defers cancelation.We should not cancel the client-scoped contexts after returning a download stream. Instead, keep the
cancel()function on the stream objects and call it whenClose()orAbort()are called. See GODRIVER-3325 for a more robust and long-term solution.Background & Motivation
Discovered dogfooding evergreen.