Skip to content
This repository has been archived by the owner on May 6, 2024. It is now read-only.

synchronize context cancellation with writing to the result channel #34

Closed
petar opened this issue May 18, 2022 · 8 comments · Fixed by #35
Closed

synchronize context cancellation with writing to the result channel #34

petar opened this issue May 18, 2022 · 8 comments · Fixed by #35
Assignees

Comments

@petar
Copy link
Collaborator

petar commented May 18, 2022

Generated client method implementations must stop writing to the result channels as soon as they see "context cancelled".
This relieves clients from the responsibility to drain the result channel after they cancel the context.

A few changes in the generated code are required:

When context cancellation is detected, an error message should not be sent to the result channel:
https://github.com/ipld/edelweiss/blob/main/blueprints/services/dagjson-over-http/client.go#L284

All writing to the result channel should be paired with synchronized checks for context cancellation:
https://github.com/ipld/edelweiss/blob/main/blueprints/services/dagjson-over-http/client.go#L293
https://github.com/ipld/edelweiss/blob/main/blueprints/services/dagjson-over-http/client.go#L299
https://github.com/ipld/edelweiss/blob/main/blueprints/services/dagjson-over-http/client.go#L304
https://github.com/ipld/edelweiss/blob/main/blueprints/services/dagjson-over-http/client.go#L310

@ajnavarro
Copy link
Member

@ajnavarro: I will take this (I cannot assign myself to the issue).

@BigLep
Copy link

BigLep commented May 18, 2022

@ajnavarro : go ahead and create a PR here to add you to w3dt-stewards team: https://github.com/ipld/github-mgmt . That should give you the ability to self-assign.

@BigLep
Copy link

BigLep commented May 18, 2022

Also, it looks like we'll need to associate this repo with w3dt-stewards team as well. That can be in the same PR.

@ajnavarro
Copy link
Member

@BigLep done here: ipld/github-mgmt#9

@guseggert
Copy link
Contributor

guseggert commented May 19, 2022

What if Edelweiss returns iterators for these collections, and only generates a method that returns an iterator, instead of sync+async? It'd remove all this confusing async machinery, spawn one less goroutine for sync calls, and if consumer wants async, they can just wrap it in a goroutine, which is a lot easier IMO since they'd own the entire lifetime of the channel and goroutine, cancellation semantics, etc. The iterator would need to be explicitly closed so that it closes the underlying HTTP response body.

@ajnavarro
Copy link
Member

@guseggert I created an issue with a proposal for using iterators instead of channels here: #38

@petar
Copy link
Collaborator Author

petar commented May 23, 2022

I am reopening this issue, as the PR does not synchronize channel sends with context cancellation. I left a note in the merged PR that explains the problem.

@petar petar reopened this May 23, 2022
@ajnavarro
Copy link
Member

Hey @petar I created a draft PR to see if it is what we are looking for, can you have a look? #40
Thanks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants