Bitswap Getblocks: CancelWant on ctx cancel #1995

rht · 2015-11-22T16:23:06Z

Though ipfs get cancel works only after the close notify issue has been fixed.

License: MIT Signed-off-by: David Dias <[email protected]>

This used to lead to large refcount numbers, causing Flush to create a lot of IPFS objects, and merkledag to consume tens of gigabytes of RAM. License: MIT Signed-off-by: Jeromy <[email protected]>

OS X sed is documented as "-i SUFFIX", GNU sed as "-iSUFFIX". The one consistent case seems to be "-iSUFFIX", where suffix cannot empty (or OS X will parse the next argument as the suffix). This used to leave around files named `refsout=` on Linux, and was just confusing. License: MIT Signed-off-by: Jeromy <[email protected]>

License: MIT Signed-off-by: Jeromy <[email protected]>

These secondary copies were never actually queried, and didn't contain the indirect refcounts so they couldn't become the authoritative source anyway as is. New goal is to move pinning into IPFS objects. A migration will be needed to remove the old data from the datastore. This can happen at any time after this commit. License: MIT Signed-off-by: Jeromy <[email protected]>

License: MIT Signed-off-by: Jeromy <[email protected]>

Pinner had method GetManual that returned a ManualPinner, so every Pinner had to implement ManualPinner anyway. License: MIT Signed-off-by: Jeromy <[email protected]>

License: MIT Signed-off-by: Jeromy <[email protected]>

Platform-dependent behavior is not nice, and negative refcounts are not very useful. License: MIT Signed-off-by: Jeromy <[email protected]>

License: MIT Signed-off-by: Jeromy <[email protected]>

License: MIT Signed-off-by: Jeromy <[email protected]> sharness: Don't assume we know all things that can create garbage License: MIT Signed-off-by: Jeromy <[email protected]>

WARNING: No migration performed! That needs to come in a separate commit, perhaps amended into this one. This is the minimal rewrite, only changing the storage from JSON(+extra keys) in Datastore to IPFS objects. All of the pinning state is still loaded in memory, and written from scratch on Flush. To do more would require API changes, e.g. adding error returns. Set/Multiset is not cleanly separated into a library, yet, as it's API is expected to change radically. License: MIT Signed-off-by: Jeromy <[email protected]>

License: MIT Signed-off-by: Jeromy <[email protected]>

…lure License: MIT Signed-off-by: Jeromy <[email protected]>

There was doublewrapping with an unneeded msgio. given that we use a stream muxer now, msgio is only needed by secureConn -- to signal the boundaries of an encrypted / mac-ed ciphertext. Side note: i think including the varint length in the clear is actually a bad idea that can be exploited by an attacker. it should be encrypted, too. (TODO) License: MIT Signed-off-by: Jeromy <[email protected]>

License: MIT Signed-off-by: Jeromy <[email protected]>

* ID service stream * make the relay service use msmux * fix nc tests Note from jbenet: Maybe we should remove the old protocol/muxer and see what breaks. It shouldn't be used by anything now. License: MIT Signed-off-by: Jeromy <[email protected]> Signed-off-by: Juan Batiz-Benet <[email protected]>

The addition of a locking interface to the blockstore allows us to perform atomic operations on the underlying datastore without having to worry about different operations happening in the background, such as garbage collection. License: MIT Signed-off-by: Jeromy <[email protected]>

This commit improves (fixes) the FetchGraph call for recursively fetching every descendant node of a given merkledag node. This operation should be the simplest way of ensuring that you have replicated a dag locally. This commit also implements a method in the merkledag package called EnumerateChildren, this method is used to get a set of the keys of every descendant node of the given node. All keys found are noted in the passed in KeySet, which may in the future be implemented on disk to avoid excessive memory consumption. License: MIT Signed-off-by: Jeromy <[email protected]>

License: MIT Signed-off-by: Jeromy <[email protected]>

License: MIT Signed-off-by: Juan Batiz-Benet <[email protected]>

License: MIT Signed-off-by: Jeromy <[email protected]> dont GC blocks used by pinner License: MIT Signed-off-by: Jeromy <[email protected]> comment GC algo License: MIT Signed-off-by: Jeromy <[email protected]> add lock to blockstore to prevent GC from eating wanted blocks License: MIT Signed-off-by: Jeromy <[email protected]> improve FetchGraph License: MIT Signed-off-by: Jeromy <[email protected]> separate interfaces for blockstore and GCBlockstore License: MIT Signed-off-by: Jeromy <[email protected]> reintroduce indirect pinning, add enumerateChildren dag method License: MIT Signed-off-by: Jeromy <[email protected]>

License: MIT Signed-off-by: Jeromy <[email protected]>

License: MIT Signed-off-by: rht <[email protected]>

Implements a solution for #1908 This PR replaces #1909 License: MIT Signed-off-by: Andrew Chin <[email protected]>

License: MIT Signed-off-by: Jeromy <[email protected]>

Add a --no-pin option to `ipfs add`

License: MIT Signed-off-by: Jeromy <[email protected]>

Fix/add mem 4

License: MIT Signed-off-by: Jeromy <[email protected]>

if bucket doesnt have enough peers, grab more elsewhere

add closenotify and large timeout to gateway

License: MIT Signed-off-by: rht <[email protected]>

Add config option for flatfs no-sync

License: MIT Signed-off-by: rht <[email protected]>

rht · 2015-11-22T16:29:04Z

exchange/bitswap/bitswap.go

@@ -208,6 +208,12 @@ func (bs *Bitswap) GetBlocks(ctx context.Context, keys []key.Key) (<-chan *block
 		log.Event(ctx, "Bitswap.GetBlockRequest.Start", &k)
 	}

+	go func() {


Although it would be better to write something that is like ctx, cancel := ContextCancelWants(ctx)

if its just a single case, you dont need the select.

What about adding a default timeout (~time.Hour*24)?

mmm... i don't know about that. maybe ask @jbenet

i'm generally against timeouts in places where they should be implemented higher up by a caller.

The lines later have <-ctx.Done(), if this is to moved here as well?

Actually, the timeout is meant for the commands in general, not specific to bitswap. ctx.Done() already includes timeout case if it is specified.

callers of blockservice.GetBlock can set timeouts as they see necessary.

(or rather, callers of the dagservice.Get)

+1 to having callers always give the proper timeouts. we really should remove remaining context.TODO and context.Background to make sure everything has a proper reason for existing.

i believe the http server stuff has ways of detecting that the client has gone away and is no longer expecting a response. it may be tricky to get the wiring to the context right, if it's not yet there.

we may want to hunt into some google http server code that uses contexts, or look in the golang-nuts mailing list.

License: MIT Signed-off-by: rht <[email protected]>

jbenet · 2015-12-01T03:16:37Z

this change in general LGTM. but the test failures are bad news :( -- need to get that fixed.

also, i think we should make sure everything that feeds into callers of bitswap (and their calllers) do so with properly cancelled contexts, all the way back to the requests causing them.

that said, there is a behavior we may want to allow, a sort of like "detached request", where we have a set of "currently being downloaded" dags that continue to be requested until the user shuts down ipfs or cancels them explicitly. Think of this like a torrent client. we could make that an explicit option in a get request or whatever, and have a facility for listing those ongoing long-term requests. (or if we do this for all requests, have a facility for manually canceling the whole dag get, not unwanting hash by hash.)

jbenet · 2016-01-24T11:59:35Z

@rht why close this PR? no longer needed? or needs a rebase?

daviddias and others added 30 commits November 11, 2015 10:04

update version

f72b675

License: MIT Signed-off-by: David Dias <[email protected]>

pin: Guard against callers causing refcount underflow

80a8af7

This used to lead to large refcount numbers, causing Flush to create a lot of IPFS objects, and merkledag to consume tens of gigabytes of RAM. License: MIT Signed-off-by: Jeromy <[email protected]>

sharness: fusermount -u is the documented way to unmount FUSE on Linux

86d781a

License: MIT Signed-off-by: Jeromy <[email protected]>

pin: unexport NewIndirectPin, it's not useful and not used elsewhere

cb46a52

License: MIT Signed-off-by: Jeromy <[email protected]>

blocks/set: Remove now-unused NewDBWrapperSet

34f8231

License: MIT Signed-off-by: Jeromy <[email protected]>

Simplify Pinner interface by folding ManualPinner into Pinner

3a05a94

Pinner had method GetManual that returned a ManualPinner, so every Pinner had to implement ManualPinner anyway. License: MIT Signed-off-by: Jeromy <[email protected]>

pin: Remove dead code

832d639

License: MIT Signed-off-by: Jeromy <[email protected]>

pin: Remove double bookkeeping of refcount keys

c65bf3f

License: MIT Signed-off-by: Jeromy <[email protected]>

Use uint64 for indirect pin refcounts

f85862b

Platform-dependent behavior is not nice, and negative refcounts are not very useful. License: MIT Signed-off-by: Jeromy <[email protected]>

Typo

99448d4

License: MIT Signed-off-by: Jeromy <[email protected]>

sharness: Don't assume we know all things that can create garbage

21e7b05

License: MIT Signed-off-by: Jeromy <[email protected]> sharness: Don't assume we know all things that can create garbage License: MIT Signed-off-by: Jeromy <[email protected]>

pin: Future-proof against refcount marshaled size changes

283432f

License: MIT Signed-off-by: Jeromy <[email protected]>

bump fsrepo version to 3

81b16e2

License: MIT Signed-off-by: Jeromy <[email protected]>

pin: Do not accidentally delete indirect pins on Flush

5db177e

License: MIT Signed-off-by: Jeromy <[email protected]>

dagmodifier: Don't lose pin if old and new key happen to be equal

fd28a39

License: MIT Signed-off-by: Jeromy <[email protected]>

dagmodifier test: Add TODO note about how bad luck can cause test fai…

1e0d72f

…lure License: MIT Signed-off-by: Jeromy <[email protected]>

buffer msgio

8ee7129

License: MIT Signed-off-by: Jeromy <[email protected]>

address concerns from PR

506c46f

License: MIT Signed-off-by: Jeromy <[email protected]>

move locking out of GC branch

06184a6

License: MIT Signed-off-by: Jeromy <[email protected]>

lock blockstore for pin add

588f53e

License: MIT Signed-off-by: Jeromy <[email protected]>

renamed {R,}Lock -> {Pin,GC}Lock

d71bb78

License: MIT Signed-off-by: Juan Batiz-Benet <[email protected]>

break up GC logic

8650e44

License: MIT Signed-off-by: Jeromy <[email protected]>

rht and others added 13 commits November 11, 2015 10:04

Move parts of ipfs add into core/coreunix

1bbc472

License: MIT Signed-off-by: rht <[email protected]>

Add a --pin option to ipfs add (allowing --pin=false)

b4c4591

Implements a solution for #1908 This PR replaces #1909 License: MIT Signed-off-by: Andrew Chin <[email protected]>

improves memory usage of add

605b24f

License: MIT Signed-off-by: Jeromy <[email protected]>

rework editor creation and finalization

e002194

License: MIT Signed-off-by: Jeromy <[email protected]>

Merge pull request #1931 from eminence/addnopin_v2

09d34db

Add a --no-pin option to `ipfs add`

comment multiple dagstore error checking

02e14c4

License: MIT Signed-off-by: Jeromy <[email protected]>

Merge pull request #1965 from ipfs/fix/add-mem-4

5f8cfc9

Fix/add mem 4

if bucket doesnt have enough peers, grab more elsewhere

06bc124

License: MIT Signed-off-by: Jeromy <[email protected]>

add closenotify and large timeout to gateway

cb56ec1

License: MIT Signed-off-by: Jeromy <[email protected]>

Merge pull request #1967 from ipfs/fix/kbucket-fill

d46c0d4

if bucket doesnt have enough peers, grab more elsewhere

Merge pull request #1980 from ipfs/fix/gateway-close-notif

856e250

add closenotify and large timeout to gateway

Add config option for flatfs no-sync

cab5868

License: MIT Signed-off-by: rht <[email protected]>

Merge pull request #1985 from ipfs/rht-add-no-sync

b4ab684

Add config option for flatfs no-sync

jbenet added the status/in-progress In progress label Nov 22, 2015

Bitswap Getblocks: CancelWant on ctx cancel

ad84e8b

License: MIT Signed-off-by: rht <[email protected]>

rht force-pushed the wire/ctx-bitswap-cancelwant branch from 5717463 to ad84e8b Compare November 22, 2015 16:24

rht reviewed Nov 22, 2015
View reviewed changes

rht added the RFCR label Nov 22, 2015

Address CR comment

f4b40ec

License: MIT Signed-off-by: rht <[email protected]>

rht force-pushed the wire/ctx-bitswap-cancelwant branch from 5ac132e to f4b40ec Compare November 23, 2015 01:00

rht mentioned this pull request Nov 23, 2015

wire contexts into bitswap requests more deeply #1986

Closed

whyrusleeping force-pushed the dev0.4.0 branch 2 times, most recently from 68b9745 to b0a8591 Compare December 28, 2015 14:36

whyrusleeping force-pushed the dev0.4.0 branch from cf514d8 to 3224ae0 Compare January 12, 2016 16:23

rht mentioned this pull request Jan 24, 2016

bitswap GetBlocks: CancelWant on ctx cancel #2238

Closed

1 task

rht closed this Jan 24, 2016

Kubuxu deleted the wire/ctx-bitswap-cancelwant branch February 27, 2017 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bitswap Getblocks: CancelWant on ctx cancel #1995

Bitswap Getblocks: CancelWant on ctx cancel #1995

rht commented Nov 22, 2015

rht Nov 22, 2015

whyrusleeping Nov 22, 2015

rht Nov 23, 2015

whyrusleeping Nov 23, 2015

whyrusleeping Nov 23, 2015

rht Nov 23, 2015

rht Nov 23, 2015

whyrusleeping Nov 25, 2015

whyrusleeping Nov 25, 2015

jbenet Dec 1, 2015

jbenet commented Dec 1, 2015

jbenet commented Jan 24, 2016

Bitswap Getblocks: CancelWant on ctx cancel #1995

Bitswap Getblocks: CancelWant on ctx cancel #1995

Conversation

rht commented Nov 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbenet commented Dec 1, 2015

jbenet commented Jan 24, 2016