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

Make closenotify work [wip] #1998

Closed

rht wants to merge 75 commits into dev0.4.0 from fix/closenotify

Contributor

rht commented Nov 23, 2015

@whyrusleeping this appears to reveal the closenotify problem, that now I can receive the "closenotify".

Looks like closenotify channel is consumed during the version check.
This PR still doesn't fix the client cancel yet, since the cancel is for version check request, not the command itself.

daviddias and others added 30 commits

November 11, 2015 10:04


          update version

f72b675

License: MIT
Signed-off-by: David Dias <daviddias.p@gmail.com>


          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 <jeromyj@gmail.com>


          sharness: Use sed in a cross-platform safe way

11185e3

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 <jeromyj@gmail.com>


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

86d781a

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>


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

cb46a52

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>


          pin: Remove code shadowing pins as datastore keys

cad9e47

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 <jeromyj@gmail.com>


          blocks/set: Remove now-unused NewDBWrapperSet

34f8231

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>


          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 <jeromyj@gmail.com>


          pin: Remove dead code

832d639

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>


          pin: Remove double bookkeeping of refcount keys

c65bf3f

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>


          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 <jeromyj@gmail.com>


          Typo

99448d4

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>


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

21e7b05

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>

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

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>


          pin: Rewrite to store pins in IPFS objects

60a505f

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 <jeromyj@gmail.com>


          pin: Future-proof against refcount marshaled size changes

283432f

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>


          bump fsrepo version to 3

81b16e2

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>


          pin: Do not accidentally delete indirect pins on Flush

5db177e

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>


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

fd28a39

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>


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

1e0d72f

…lure

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>


          remove msgio double wrap

b45d248

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 <jeromyj@gmail.com>


          buffer msgio

8ee7129

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>


          using multistream muxer

b4754c8

* 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 <jeromyj@gmail.com>
Signed-off-by: Juan Batiz-Benet <juan@benet.ai>


          Add locking interface to blockstore

41b58e1

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 <jeromyj@gmail.com>


          merkledag FetchGraph and EnumerateChildren

ae3a453

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 <jeromyj@gmail.com>


          address concerns from PR

506c46f

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>


          move locking out of GC branch

06184a6

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>


          lock blockstore for pin add

588f53e

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>


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

d71bb78

License: MIT
Signed-off-by: Juan Batiz-Benet <juan@benet.ai>


          implement mark and sweep GC

d0aa03c

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>

dont GC blocks used by pinner

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>

comment GC algo

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>

add lock to blockstore to prevent GC from eating wanted blocks

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>

improve FetchGraph

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>

separate interfaces for blockstore and GCBlockstore

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>

reintroduce indirect pinning, add enumerateChildren dag method

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>


          break up GC logic

8650e44

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>

whyrusleeping and others added 11 commits

November 18, 2015 23:19


          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 <jeromyj@gmail.com>


          add closenotify and large timeout to gateway

cb56ec1

License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>


          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 <rhtbot@gmail.com>


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

b4ab684

Add config option for flatfs no-sync


          Directly wire ctx into http request

a2e3c69

License: MIT
Signed-off-by: rht <rhtbot@gmail.com>


          Cleanup http client Send

54604c9

License: MIT
Signed-off-by: rht <rhtbot@gmail.com>


          Simplify context canceled err message

3109b59

License: MIT
Signed-off-by: rht <rhtbot@gmail.com>


          Make closenotify work

103fac6

License: MIT
Signed-off-by: rht <rhtbot@gmail.com>

jbenet added the status/in-progress label

Contributor Author

rht commented Nov 23, 2015

golang/go#13165

Member

whyrusleeping commented Nov 23, 2015

@rht interesting, so if that bug is what we're seeing, then we could solve the problem by adding some buffering to the read side of the request body.

Contributor Author

rht commented Nov 23, 2015

How to buffer the ResponseWriter's channel without rewriting how ServeHTTP is called?

Or put the version check and the command into 1 request packet, fixing #1990 along the way.

Member

whyrusleeping commented Nov 25, 2015

hrm, i think this should be able to be solved without doing anything hacky. We just have to figure out what we're doing thats breaking it. Think you could try and get a minimal repro carved out?

Member

whyrusleeping commented Nov 26, 2015

@rht close notify is high priority now, i'm noticing it causing lots of issues. Do you think you can figure something out by this weekend? if not, i'll prioritize taking it on myself.

Contributor Author

rht commented Nov 26, 2015

How about If I figure a sketch of how to solve it I will update you right away?

Contributor Author

rht commented Nov 26, 2015

(I think this has been solved in etcd before; while golang solution will have to wail until go1.6)

Member

whyrusleeping commented Nov 26, 2015

@rht any idea why ours doesnt work, but a simple http server using close notify succeeds?

Member

whyrusleeping commented Nov 26, 2015

How about If I figure a sketch of how to solve it I will update you right away?

Sounds good to me, keep me posted

Contributor Author

rht commented Nov 26, 2015

@whyrusleeping I tried catching the <-ctx.Done() in commands/http/client.client.Send, explicitly, I added

go func(){
    <-req.Context().Done()
    log.Error("client ctx is really canceled")
}()

before httpReq.Cancel = req.Context().Done().

It doesn't print any log message when I C-c the client.

Edit: nvm, I have to additionally check if the chan is closed.

Contributor Author

rht commented Nov 27, 2015

((Saw the 'closenotify' firing when the client's transport's DisableKeepAlive was set to false (and other tweaks). When I tried to isolate this change, no longer saw the closenotify ever again))

ghost added the kind/bug label

whyrusleeping force-pushed the dev0.4.0 branch 2 times, most recently from 68b9745 to b0a8591 Compare

December 28, 2015 14:36

whyrusleeping closed this

jbenet removed the status/in-progress label

Member

whyrusleeping commented Dec 29, 2015

will be fixed in #2032

whyrusleeping deleted the fix/closenotify branch

December 29, 2015 16:11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels