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

fix close notify #2032

Merged
merged 5 commits into from
Dec 29, 2015
Merged

fix close notify #2032

merged 5 commits into from
Dec 29, 2015

Conversation

whyrusleeping
Copy link
Member

Our multipart file code cannot handle sending zero files. So if the number of input files is nil, don't do any multipart stuff.

Also did some other cleanup along the way to make debugging easier.

@jbenet jbenet added the status/in-progress In progress label Dec 3, 2015
@whyrusleeping
Copy link
Member Author

hrm... the test i'm writing will be a LOT easier with #2003. I'm trying to use netcat to capture the http request, but the version check goes first and i capture that instead...

@rht
Copy link
Contributor

rht commented Dec 3, 2015

This surely fixes the closenotify problem, but not entirely obvious why.
The first roundtrip circle is somehow completed.

@rht
Copy link
Contributor

rht commented Dec 3, 2015

#2003 is currently on dev0.4.0

@whyrusleeping
Copy link
Member Author

@rht the close notify issue appeared to be because the client was sending a malformed http request body. That caused the server side to mess up for some reason and not figure out how the close notify should happen for some reason.

} else {
// if we have no file data, use an empty Reader
// (http.NewRequest panics when a nil Reader is used)
reader = strings.NewReader("")
Copy link
Member

Choose a reason for hiding this comment

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

is this no longer true, then?

Copy link
Member Author

Choose a reason for hiding this comment

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

it was actually never true, as far as i can tell, at least since april 2014.

@jbenet
Copy link
Member

jbenet commented Dec 3, 2015

this LGTM in general.

@whyrusleeping maybe write a proxy that checks the contents are fine, but still fwds the request to something else? not sure.

@whyrusleeping
Copy link
Member Author

maybe write a proxy that checks the contents are fine, but still fwds the request to something else? not sure.

@jbenet i was going to do that with netcat, but it will be difficult before #2003

I think I might just merge the test i have in after that one lands

@jbenet
Copy link
Member

jbenet commented Dec 4, 2015

@jbenet i was going to do that with netcat, but it will be difficult before #2003

that's why "proxy that checks the contents are fine, but still fwds the request to something else" -- a proxy would not have a problem with the version check, it would go through it fine.

@whyrusleeping
Copy link
Member Author

I mean, if we want to have another bit of go code to do that we can. But at that point, we might as well just test it exclusively in go

@jbenet
Copy link
Member

jbenet commented Dec 4, 2015

ok that's fine, as long as it is a real test (testing with the full api service running).

fwiw, having a tool that can proxy + verify certain things pass through will be useful anyway. imagine something like:

cat testfile
> grep ^Hello Server$
< grep ^Hello Client$
> grep ^Here are [0-9]+ things$
+> grep $Thing [0-9]+$
> grep ^Done$
< grep ^Ok got [0-9]+ things

cat testfile | testproxy --port 5679 1.2.3.4:5678

this may be overkill for now though :)

@jbenet jbenet mentioned this pull request Dec 7, 2015
14 tasks
@rht
Copy link
Contributor

rht commented Dec 16, 2015

For the test, #2003 -> #2037.

@ghost ghost added the kind/bug A bug in existing code (including security flaws) label Dec 22, 2015
License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
License: MIT
Signed-off-by: Jeromy <jeromyj@gmail.com>
@cryptix
Copy link
Contributor

cryptix commented Dec 28, 2015

LGTM

@whyrusleeping
Copy link
Member Author

this has been long in coming and is actively causing issues, merging.

whyrusleeping added a commit that referenced this pull request Dec 29, 2015
@whyrusleeping whyrusleeping merged commit 8d48163 into master Dec 29, 2015
@jbenet jbenet removed the status/in-progress In progress label Dec 29, 2015
@Kubuxu Kubuxu deleted the fix/close-notify branch May 13, 2016 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants