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

make new stream calls accept a context #8

Merged
merged 1 commit into from
Nov 20, 2015
Merged

Conversation

whyrusleeping
Copy link
Contributor

this will allow dials caused by NewStream calls to be cancelled. They have a bad habit of hanging otherwise.

detectrace "github.com/jbenet/go-detect-race"
context "gx/QmacZi9WygGK7Me8mH53pypyscHzU386aUZXpr28GZgUct/context"
Copy link
Contributor

Choose a reason for hiding this comment

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

why cant this be ipfs/? or gx/ipfs/ ? (preserving paths is important. adding ipns support and other things will be possible.

please please avoid breaking protocol naming. this is the kinda crap that makes it hard to nest + reuse protocols.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

packages will never use ipns paths. that breaks the whole point of using hashes for package references. ipns names can be used for repos just fine, as those are expected to be mutable.

if a package ref were tied to an ipns hash, then installing the same package two different times may result in a broken package, as the entry may update in between installs. I can switch it to just ipfs/ instead of gx/, david and i thought that gx would be a nice prefix for branding and for making it easy to figure out why i have random hash directories sitting around different places.

Copy link
Contributor

Choose a reason for hiding this comment

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

packages will never use ipns paths. that breaks the whole point of using hashes for package references.

not necessarily. there could be other protocols.

if a package ref were tied to an ipns hash, then installing the same package two different times may result in a broken package, as the entry may update in between installs.

agreed, that's how all of Go works today

I can switch it to just ipfs/ instead of gx/, david and i thought that gx would be a nice prefix for branding and for making it easy to figure out why i have random hash directories sitting around different places.

i dont think it should be gx/hash. other protocols may be needed. i think it should be either gx/ipfs/hash (proper nesting) or just ipfs/hash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@noffle @diasdavid, thoughts here? i'm amenable to changing things to ipfs/hash but too much testing is less fun.

Copy link
Member

Choose a reason for hiding this comment

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

gx/ipfs/hashcould be good when we start "mounting everything" and just let the OS figure out someone is requesting for a ipfs link and fetches it from ipfs. Having it gx is kind of nice because keeps things organised on the local fs (+it is really good for devs to look and tell first hand "oh, this uses gx to fetch the dependency)

Copy link
Contributor

Choose a reason for hiding this comment

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

gx/ doesn't encode protocol information re retrieval; @jbenet makes a good point here. However, the import path here doesn't need to encode protocol information. @whyrusleeping, I think we talked about something like a .git directory (so, .gx) that stored each package's local installation FS path (gx/<hash> here). And then the package.json would define that package's protocol path (e.g. /ipns/<hash>). My only qualm with adding another layer of nesting is that diminishes the user experience (try using command line tools in Java projects -- ha).

gx/[ipfs|ipns]/: encodes the protocol underneath the gx/ prefix. Nice that it matches up with what's in package.json, but not necessary (as per above). I like the 1:1 matching of FS path and location here.

I don't really like ipfs/hash/, since drops the gx substring. Having it might be nice for identifying what this folder means when looking at a project using gx.

Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't need to encode protocol information

this breaks encapsulation and proper layering. it doesnt need another
protocol today, but it makes it way harder for us to evolve gx tomorrow --
supposing we had some other prefix (either someone else's protocol, or even
an ipfs protocol that made sense). embedding the assumption that gx/ always
means
ipfs/ is a source of issues. either allow full paths, or gx-nested
full paths, but not gx-nested partial paths.

On Fri, Nov 20, 2015 at 3:39 PM Stephen Whitmore notifications@github.com
wrote:

In p2p/net/mock/mock_test.go
#8 (comment):

detectrace "github.com/jbenet/go-detect-race"
  • context "gx/QmacZi9WygGK7Me8mH53pypyscHzU386aUZXpr28GZgUct/context"

gx/ doesn't encode protocol information re retrieval; @jbenet
https://github.com/jbenet makes a good point here. However, the import
path here doesn't need to encode protocol information. @whyrusleeping
https://github.com/whyrusleeping, I think we talked about something
like a .git directory (so, .gx) that stored each package's local
installation FS path (gx/ here). And then the package.json would
define that package's protocol path (e.g. /ipns/). My only qualm
with adding another layer of nesting is that diminishes the user experience
(try using command line tools in Java projects -- ha).

gx/[ipfs|ipns]/: encodes the protocol underneath the gx/ prefix. Nice
that it matches up with what's in package.json, but not necessary (as per
above). I like the 1:1 matching of FS path and location here.

I don't really like ipfs/hash/, since drops the gx substring. Having it
might be nice for identifying what this folder means when looking at a
project using gx.


Reply to this email directly or view it on GitHub
https://github.com/ipfs/go-libp2p/pull/8/files#r45533043.

jbenet added a commit that referenced this pull request Nov 20, 2015
make new stream calls accept a context
@jbenet jbenet merged commit e0643c7 into master Nov 20, 2015
@jbenet jbenet deleted the feat/new-stream-ctx branch November 20, 2015 02:57
@whyrusleeping
Copy link
Contributor Author

please just give a LGTM and let me merge.

@jbenet
Copy link
Contributor

jbenet commented Nov 20, 2015

@whyrusleeping it's been merged for an hour

marten-seemann pushed a commit that referenced this pull request Jan 3, 2022
use 6hrs as ttl for routing based advertisements
marten-seemann pushed a commit that referenced this pull request Jan 9, 2022
use 6hrs as ttl for routing based advertisements
marten-seemann pushed a commit that referenced this pull request Apr 21, 2022
Introduce changes required for Private Netowrk support
marten-seemann pushed a commit that referenced this pull request Apr 22, 2022
Compatibility with the new transport interfaces
marten-seemann pushed a commit that referenced this pull request Aug 17, 2022
Avoids circular module dependency.
marten-seemann pushed a commit that referenced this pull request Aug 29, 2023
Reset streams when no reading is going to be done anymore
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.

4 participants