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

Send empty struct to pubsub cmd output to flush #3416

Merged
merged 1 commit into from
Nov 25, 2016

Conversation

keks
Copy link
Contributor

@keks keks commented Nov 23, 2016

So the HTTP headers get sent

@Kubuxu Kubuxu added the status/in-progress In progress label Nov 23, 2016
n := binary.PutUvarint(buf, uint64(len(m.Data)))
return io.MultiReader(bytes.NewReader(buf[:n]), bytes.NewReader(m.Data)), nil
var (
n int
Copy link
Member

Choose a reason for hiding this comment

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

I dislike this syntax, lets just instantiate things where they are used.

Also, no need for the m.Message != nil check, its functionally equivalent to just saying data := m.Message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea here is that data is nil if m.Message is nil.

Copy link
Member

Choose a reason for hiding this comment

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

Yeap, i know

@@ -118,16 +122,33 @@ To use, the daemon must be run with '--enable-pubsub-experiment'.
},
Marshalers: cmds.MarshalerMap{
cmds.Text: getPsMsgMarshaler(func(m *floodsub.Message) (io.Reader, error) {
if m.Message == nil {
Copy link
Member

Choose a reason for hiding this comment

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

You can leave this out too, passing a nil byte array to a bytes.NewReader will result in the same as passing the empty string to a stringsreader

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I can't. We use m.Data later, which is m.Message.Data, which is a nil dereference.

Copy link
Member

Choose a reason for hiding this comment

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

If m.Message is nil, what is the value of m.Data ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Message is embedded and Data is part of Message.

Copy link
Contributor Author

@keks keks Nov 23, 2016

Choose a reason for hiding this comment

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

and if m.Message is nil, m.Message.Data panics - so does m.Data

return bytes.NewReader(m.Data), nil
}),
"ndpayload": getPsMsgMarshaler(func(m *floodsub.Message) (io.Reader, error) {
if m.Message == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Same here, append([]byte(nil), '\n') works just fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And where would I store that? m.Data does not exist because it is actually m.Message.Data, and m.Message is nil. Panic!

@keks
Copy link
Contributor Author

keks commented Nov 23, 2016

Oh and m.Message is nil when sending the empty message

So the HTTP headers get sent

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

@keks sorry about that, I missed the fact that there was an embedded struct involved.

@haadcode
Copy link
Member

I endorse this PR and/or service. Would be awesome to get this into 0.4.5-rc1 build.

@keks
Copy link
Contributor Author

keks commented Nov 24, 2016

@whyrusleeping can you merge? I'll look at sharness tests next week.

@whyrusleeping whyrusleeping merged commit 0980bf6 into ipfs:master Nov 25, 2016
@whyrusleeping whyrusleeping removed the status/in-progress In progress label Nov 25, 2016
@ghost ghost mentioned this pull request Dec 23, 2016
@keks keks deleted the feat/instaflush branch November 22, 2017 18:41
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