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

http api: makes sure header is sent even when r is not ready yet. fixes #3304 #3305

Merged
merged 4 commits into from
Oct 18, 2016

Conversation

keks
Copy link
Contributor

@keks keks commented Oct 15, 2016

Otherwise API clients block on pubsub subscriptions until the first packet is sent (see, well, #3304).

License: MIT
Signed-off-by: Jan Winkelmann <j-winkelmann@tuhh.de>
Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

Great catch, this should fix quite a few 'apparent' hanging issues. Just one change then LGTM

@@ -298,6 +298,9 @@ func flushCopy(w io.Writer, r io.Reader) error {
return err
}
for {
// flush to send header when r is not ready yet
f.Flush()
Copy link
Member

Choose a reason for hiding this comment

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

probably want to check and handle the error here, just in case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flush doens't return an error, that's why the tests failed :D

License: MIT
Signed-off-by: Jan Winkelmann <j-winkelmann@tuhh.de>
@keks
Copy link
Contributor Author

keks commented Oct 15, 2016

I now check the error. Also I removed the flush at the bottom of the loop so it doesn't flush pointlessly.
Now it also flushes when the reader is drained (err, n = r.Read(); err == io.EOF && n <= 0)

@whyrusleeping
Copy link
Member

Tests are failing now... i'm guessing removing that final flush wasnt quite right

License: MIT
Signed-off-by: Jan Winkelmann <j-winkelmann@tuhh.de>
License: MIT
Signed-off-by: Jan Winkelmann <j-winkelmann@tuhh.de>
@Kubuxu
Copy link
Member

Kubuxu commented Oct 18, 2016

Can you squash it, we don't want to have any commits with build/test failures as it makes git bisecting much harder.

@whyrusleeping whyrusleeping merged commit 68d8a29 into ipfs:master Oct 18, 2016
@keks
Copy link
Contributor Author

keks commented Oct 19, 2016

@Kubuxu uh, sorry. next time!

keks added a commit to keks/go-ipfs that referenced this pull request Nov 7, 2016
keks added a commit to keks/go-ipfs that referenced this pull request Nov 7, 2016
keks added a commit to keks/go-ipfs that referenced this pull request Nov 7, 2016
…yet. fixes ipfs#3304 (ipfs#3305)"

This reverts commit 68d8a29.

License: MIT
Signed-off-by: Jan Winkelmann <j-winkelmann@tuhh.de>
keks added a commit to keks/go-ipfs that referenced this pull request Nov 7, 2016
Revert "http api: makes sure header is sent even when r is not ready yet. fixes ipfs#3304 (ipfs#3305)"
This reverts commit 68d8a29.

License: MIT
Signed-off-by: Jan Winkelmann <j-winkelmann@tuhh.de>

License: MIT
keks added a commit to keks/go-ipfs that referenced this pull request Nov 7, 2016
Revert "http api: makes sure header is sent even when r is not ready yet. fixes ipfs#3304 (ipfs#3305)"
This reverts commit 68d8a29.

License: MIT
Signed-off-by: Jan Winkelmann <j-winkelmann@tuhh.de>
liliuhai added a commit to peersafe/go-ipfs that referenced this pull request Dec 26, 2016
* commit '6f3ae5da293f971c09e3e6fc0763aeca04ba98d3':
  rename DataSize to FileSize
  update HashOnRead validation to properly support cids
  http api: makes sure header is sent even when r is not ready yet. fixes ipfs#3304 (ipfs#3305)
  fix add/cat of small files
  raw dag: make raw nodes work in cat and get, add tests
  feat: make metrics injection log an error instead of warning
  test: check if metrics work in sharness
  Fix metrics being injected after node initalization
  unixfs: allow use of raw merkledag nodes for unixfs files
  ds-help: add helper func to convert from Cid to DsKey and the reverse
  cli: refactor to expose argument parsing functionality

# Conflicts:
#	core/commands/add.go
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.

3 participants