search

ipld / edelweiss

Decentralized Protocol Compiler
Other
16 stars 6 forks source link

synchronize context cancellation with writing to the result channel #34

Closed petar closed 2 years ago

petar commented 2 years ago

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 commented 2 years ago

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

BigLep commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

@BigLep done here: https://github.com/ipld/github-mgmt/pull/9

guseggert commented 2 years ago

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 commented 2 years ago

@guseggert I created an issue with a proposal for using iterators instead of channels here: https://github.com/ipld/edelweiss/issues/38

petar commented 2 years ago

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.

ajnavarro commented 2 years ago

Hey @petar I created a draft PR to see if it is what we are looking for, can you have a look? https://github.com/ipld/edelweiss/pull/40 Thanks.