Draft
Conversation
* 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 <hacdias@gmail.com> Co-authored-by: Marcin Rataj <lidel@lidel.org>
It makes the code quite more complex and there is no evidance it helps for anything.
c156544 to
952f135
Compare
Codecov Report
@@ 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
|
lidel
reviewed
Apr 1, 2023
Member
There was a problem hiding this comment.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.IPFSBackendrequests (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 theClose () errorfunction, however withGetthis can't be done because there is no closer associated with the directoryMetadata.Bonus problem
Additionally, even if
directoryMetadatahad 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 implementationsGetfunction 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.IPFSBackendI ended up (at least for now) doing this by: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() errorfor most functions I couldn't do anything forGetso I usedruntime.SetFinalizerfor 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?