Depends on D5011
Adding terminate function to the blob put client.
Details
Details
backup builds
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| services/backup/blob_client/src/put_client.rs | ||
|---|---|---|
| 261–263 ↗ | (On Diff #16172) | Why do we return okay if the function failed to do what it was supposed to? |
| 267 ↗ | (On Diff #16172) | why do we drop here? this is highly unusual. |
| 274–277 ↗ | (On Diff #16172) | why use Err in one place, but report_error in another? |
| 280–283 ↗ | (On Diff #16172) | we checked this at the beginning of the method, what could have changed. Adding some comments to the function would help with determining what the intention of the method is. |
| 284 ↗ | (On Diff #16172) | still really don't like this checking an error state from a global variable. |
| services/backup/blob_client/src/put_client.rs | ||
|---|---|---|
| 261–263 ↗ | (On Diff #16172) | This is a valid point, we should check for error once again here before returning Ok, thanks! |
| 267 ↗ | (On Diff #16172) | I explained it to Tomek already in a different revision. Usually, you don't use drop explicitly but here we need to use it. The point is, that we want to terminate the receiver and then join the receiver thread. Since the sender is in the global state, we have to drop it explicitly as it will not go out of scope itself (and without this drop, the receiver thread will never join as the receiver will hang). |
| 274–277 ↗ | (On Diff #16172) | Oh, right, we should return Err, thanks. |
| 280–283 ↗ | (On Diff #16172) | If the first check passed, it means that the client's already been initialized. In the meantime, we performed "deinitialization" so here is a sanity check that the client is properly released. |
| 284 ↗ | (On Diff #16172) | |
| services/backup/blob_client/src/put_client.rs | ||
|---|---|---|
| 267 ↗ | (On Diff #16172) |
Can't we just move the whole closure, and then use something like spawn_blocking, and then tokio can just drop the whole closure with the channels once they're no longer useful? I would really like to avoid management of these resources as it feels like we can just re-align the resource usage to work alongside rust's lifetime model. |
| services/backup/blob_client/src/put_client.rs | ||
|---|---|---|
| 267 ↗ | (On Diff #16172) | Sorry, but I don't really understand what you mean. If you could provide an example code, that would be cool. I don't see your idea. Btw, https://stackoverflow.com/questions/31391791/closing-a-channel-like-in-go I still think that using drop here makes sense, is not dangerous, etc. I don't see why it's so bad to use in this case. But I'm waiting for your code. |
Comment Actions
I created https://linear.app/comm/issue/ENG-1754/remove-need-to-manage-client-handles for myself to investigate later.
I still think that using drop here makes sense, is not dangerous, etc. I don't see why it's so bad to use in this case. But I'm waiting for your code.
It's only necessary because retain a global reference to it. I don't think we need to actually do this, if we separate resource concerns into their respective closures. I can investigate this later.
Essentially we create the environment in which it's necessary to do drop(), when we might be able to remove the need to keep around such details.