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.

Sign up for GitHub

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

Jump to bottom

wire contexts into bitswap requests more deeply #1986

Closed
wants to merge 335 commits into from
Closed

wire contexts into bitswap requests more deeply #1986

wants to merge 335 commits into from

Conversation

whyrusleeping
Copy link
Member

This should address the issue of wantlist entries persisting in the wantlist and not being cancelled when things like 'ipfs get' are killed early

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

@jbenet jbenet added the status/in-progress In progress label Nov 20, 2015
rht
rht reviewed Nov 21, 2015
View reviewed changes
exchange/bitswap/bitswap.go
req := &blockRequest{
keys: keys,
ctx: ctx,
// NB: Optimization. Assumes that providers of key[0] are likely to
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I thought broadcasting every blocks independently is too granular considering the bandwidth cost & rtt. This reduces dup subsequent blocks download as well (except that within the first 10s, there will be duplicated download chains)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be separated out into another PR.

@rht
Copy link
Contributor

rht commented Nov 21, 2015

Tested, the result is wantlist doesn't get cleared after client exit.

  • ipfs daemon &
  • ipfs bitswap stat no wantlist
  • ipfs get $sintel
  • ipfs bitswap stat in another terminal, contains the hash
  • C-c ipfs get
  • ipfs bitswap stat wantlist contains the hash
  • ipfs bitswap stat >10s later wantlist still contains the hash

@rht
Copy link
Contributor

rht commented Nov 22, 2015

I tried catching the ctx.Done() to print the message, inside the Run phase of ipfs get (rht@8d1ea86).

The msg only gets printed when the daemon exits, but not when the client exits.
This would mean any pending request by a client won't ever be canceled?

@whyrusleeping
Copy link
Member Author

@rht, thanks for investigating. I dont think the context from the api request is being passed down to the getblocks requst properly. I need to look at that wiring a little more closely.

@whyrusleeping
Copy link
Member Author

it looks like closenotify isnt working for some reason... not sure why

@rht
Copy link
Contributor

rht commented Nov 22, 2015

Not sure the reason with why another transport is created just to cancel the client request here.

When I replaced it with the transport created previously, I was able to C-c the client once. Currently it has to be done twice. Is this a bug or feature?

@rht rht mentioned this pull request Nov 22, 2015
Closed
@rht
Copy link
Contributor

rht commented Nov 22, 2015

test: I put a 1s timeout on the http handler that auto-cancel the connection. The wantlist still doesn't get gc-ed.

@whyrusleeping
Copy link
Member Author

this is waiting until we sort out the CloseNotify nonsense.

@rht
Copy link
Contributor

rht commented Nov 23, 2015

I tried simulating the closenotify, but in this PR, the wantlist still doesn't get gc-ed.
It does, however, in #1995.

@jbenet
Copy link
Member

jbenet commented Nov 30, 2015

would be awesome to get this figured out-- am sure lots of traffic happens because of these lingering ctxs

@ghost ghost added the topic/bitswap Topic bitswap label Dec 22, 2015
@whyrusleeping whyrusleeping force-pushed the dev0.4.0 branch 2 times, most recently from 68b9745 to b0a8591 Compare December 28, 2015 14:36
@rht rht mentioned this pull request Jan 24, 2016
Closed
1 task
@whyrusleeping whyrusleeping self-assigned this Jan 29, 2016
RichardLitt and others added 10 commits January 29, 2016 16:51
@RichardLitt
Added quotes around block put command
7758ad3 
License: MIT
Signed-off-by: Richard Littauer <[email protected]>
@whyrusleeping
Merge pull request #2265 from ipfs/docs/block-put-quotes
eb9d072 
Added quotes around block put command
@rht
Directly wire ctx into http request
4599994 
License: MIT
Signed-off-by: rht <[email protected]>
@rht
Cleanup http client Send
72e98de 
License: MIT
Signed-off-by: rht <[email protected]>
@rht
Simplify context canceled err message
6f76129 
License: MIT
Signed-off-by: rht <[email protected]>
@rht
Wire ctx to getdag operations in gc.GC
fdba13a 
License: MIT
Signed-off-by: rht <[email protected]>
@RichardLitt
Added quotes to swarm and ping descriptions
ec0c3ac 
Also fixes some grammar in pings.

See #2265 (comment)

License: MIT
Signed-off-by: Richard Littauer <[email protected]>
@whyrusleeping
initial vendoring of libp2p outside of the repo with gx
0e312f5 
License: MIT
Signed-off-by: Jeromy <[email protected]>
@whyrusleeping
package.json cleanup
e12a56d 
License: MIT
Signed-off-by: Jeromy <[email protected]>
@whyrusleeping
add makefile rule for grabbing gx and deps
3b784b5 
License: MIT
Signed-off-by: Jeromy <[email protected]>
hackergrrl and others added 24 commits February 29, 2016 16:05
@hackergrrl
Prevents 'ipfs name publish' when /ipns is mounted.
0afbd18 
License: MIT
Signed-off-by: Stephen Whitmore <[email protected]>
@hackergrrl
uses iptb exclusively for mount-publish test
b4bd604 
License: MIT
Signed-off-by: Stephen Whitmore <[email protected]>
@hackergrrl
trailing whitespace
b66e089 
License: MIT
Signed-off-by: Stephen Whitmore <[email protected]>
@hackergrrl
t0031: ensure iptb init happens
f4367f6 
License: MIT
Signed-off-by: Stephen Whitmore <[email protected]>
@whyrusleeping
Merge pull request #2323 from noffle/no_publish_while_mounted
9ab67a3 
Prevents 'ipfs name publish' when /ipns is mounted.
@whyrusleeping
Merge pull request #2403 from noffle/ipfs-files-truncate
cdddf0f 
makes 'ipfs files write' usage docs clearer
@RichardLitt
Remove period from dns text examples
7227386 
It is not useful. See ipfs-inactive/http-api-spec#41 (comment)

License: MIT
Signed-off-by: Richard Littauer <[email protected]>
@RichardLitt
Added an as to ensure good English
512b25c 
License: MIT
Signed-off-by: Richard Littauer <[email protected]>
@RichardLitt
Change language for files read slightly
0d89ae2 
License: MIT
Signed-off-by: Richard Littauer <[email protected]>
@whyrusleeping
Merge pull request #2421 from ipfs/docs/dns-example-remove-period
adf8979 
Remove period from dns text examples
@whyrusleeping
use makes realpath so its more crossplatform
aa6db5d 
License: MIT
Signed-off-by: Jeromy <[email protected]>
@RichardLitt
Add default to dns resolve
8cce0aa 
See ipfs-inactive/http-api-spec#41 (comment)

License: MIT
Signed-off-by: Richard Littauer <[email protected]>
@whyrusleeping
Merge pull request #2423 from ipfs/docs/files-read-add-byte
922dc20 
Change language for files read slightly
@whyrusleeping
Merge pull request #2422 from ipfs/docs/config-add-as
d2b9ff4 
Added an `as` to ensure good English
@whyrusleeping
Merge pull request #2416 from noffle/readme_plusplus
3ee1278 
Makes install from source instructions clearer.
@whyrusleeping
Merge pull request #2424 from ipfs/docs/dns-resolve-default
a551991 
Add default to dns resolve
@whyrusleeping
Merge pull request #2100 from jedahan/smarter-launchd-install
22fcd18 
More robust version checking
@whyrusleeping
introduce low memory flag
7d0c46a 
License: MIT
Signed-off-by: Jeromy <[email protected]>
@whyrusleeping
remove trailing whitespace
64c6943 
License: MIT
Signed-off-by: Jeromy <[email protected]>
@whyrusleeping
Merge pull request #2399 from ipfs/fix/assert-gopath
6d4da61 
assert that you must build from inside your gopath
@RichardLitt
Fix spelling error
96204b5 
My bad. Introduced in #2366

License: MIT
Signed-off-by: Richard Littauer <[email protected]>
@whyrusleeping
Merge pull request #2432 from ipfs/docs/spelling-error
16c08a3 
Fix spelling error
@whyrusleeping
Merge pull request #2429 from ipfs/feat/low-mem-experiment
9a923b4 
introduce a low memory flag
@whyrusleeping
wire contexts into bitswap requests more deeply
11685f0 
License: MIT
Signed-off-by: Jeromy <[email protected]>
@whyrusleeping whyrusleeping force-pushed the fix/bitswap-ctx-wire branch from 7f04b1b to 11685f0 Compare March 3, 2016 06:37
@whyrusleeping whyrusleeping deleted the fix/bitswap-ctx-wire branch March 3, 2016 06:37
@whyrusleeping whyrusleeping restored the fix/bitswap-ctx-wire branch March 3, 2016 06:37
@whyrusleeping whyrusleeping mentioned this pull request Mar 3, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@whyrusleeping whyrusleeping

Labels
need_tests status/in-progress In progress topic/bitswap Topic bitswap
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@whyrusleeping @rht @jbenet @chriscool @hackergrrl @RichardLitt @Kubuxu @atomgardner @noffle @rwcarlsen