Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: expose closer for GetResponse #241

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

aschmahmann
Copy link
Contributor

This is more meant as a discussion then necessarily for merging as is.

High level problem

Given that we return interfaces that dynamically pull data for some of the gateway.IPFSBackend requests (e.g. Get) the implementation servicing these requests might want to know when a given interface is done being used so it can deal with any wrap up related to the request. This is doable for most requests by overriding the Close () error function, however with Get this can't be done because there is no closer associated with the directoryMetadata.

Bonus problem

Additionally, even if directoryMetadata had a close function it wouldn't be possible for middleware to add in an additional close function to the loop (e.g. an implementation that wraps another implementations Get function and wants to add a close function).

Specific issue

Listing so as to allow addressing of https://en.wikipedia.org/wiki/XY_problem concerns, and encourage alternative solutions if anyone has a better idea.

In implementing a CAR (and blocks) retrieval based implementation of gateway.IPFSBackend I ended up (at least for now) doing this by:

  1. Asynchronously starting a fetch for the relevant blocks + CARs
  2. Using a BlocksGateway internally which is back by a blockservice
  3. Feeding the blocks received over the network into the blockservice

In order for the passed in blockservice to be able to get blocks from lots of different inbound graph requests it needs to communicate with those other requests. However, we don't want that communication to continue once the HTTP request is done. While I was able to overload Close() error for most functions I couldn't do anything for Get so I used runtime.SetFinalizer for now which isn't great ipfs-inactive/bifrost-gateway#61 (comment).

Solution

The draft solution here is one of a family of related ones, but basically the idea is being able to set a close function that will get called when the response is done being used.

Alternatives

Assume the channel will either be closed due to finishing, or will have a context cancellation. Wait on a goroutine for directory listing closures. This would work, but not using a goroutine would be nice.

Also, either way we need to solve the bonus problem of allowing users access to the internals (e.g. exposing the fields, adding getters, adding modifer functions, etc.)

@lidel @hacdias thoughts, suggestions?

aschmahmann and others added 4 commits March 29, 2023 13:55
* refactor gateway api to operate on semantics to closer match user requests
* rename gateway api interface to IPFSBackend
* add blocks gateway implementation to the gateway package
* refactored gateway examples
* add default DNS resolvers
* add default IPLD codecs

---------

Co-authored-by: Henrique Dias <[email protected]>
Co-authored-by: Marcin Rataj <[email protected]>
It makes the code quite more complex and there is no evidance it helps for anything.
@aschmahmann aschmahmann force-pushed the feat/gateway-getresponse-closer branch from c156544 to 952f135 Compare March 30, 2023 16:51
@codecov
Copy link

codecov bot commented Mar 30, 2023

Codecov Report

Merging #241 (952f135) into main (6f324be) will increase coverage by 0.26%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #241      +/-   ##
==========================================
+ Coverage   47.70%   47.96%   +0.26%     
==========================================
  Files         266      269       +3     
  Lines       32950    32990      +40     
==========================================
+ Hits        15718    15824     +106     
+ Misses      15549    15500      -49     
+ Partials     1683     1666      -17     
Impacted Files Coverage Δ
gateway/handler_unixfs_dir.go 62.50% <0.00%> (-0.40%) ⬇️
gateway/handler_unixfs__redirects.go 36.64% <33.33%> (-0.45%) ⬇️
gateway/gateway.go 85.29% <57.14%> (-3.24%) ⬇️
gateway/handler_defaults.go 31.92% <100.00%> (+0.41%) ⬆️

... and 21 files with indirect coverage changes

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Late on my end, but quick thought: stumbled upon this problem in #241 and defer getResp.Close() proposed here feels like the lesser evil, or maybe even a way of simplifying code (instead of closing file.File here we would be doing close on parent struct etc), a bit easier to reason about than goroutine conventions/assumptions.

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

Successfully merging this pull request may close these issues.

3 participants