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 hijack dial #53

Merged
merged 1 commit into from
Jan 6, 2014
Merged

Fix hijack dial #53

merged 1 commit into from
Jan 6, 2014

Conversation

benmccann
Copy link
Contributor

No description provided.

@benmccann benmccann closed this Jan 5, 2014
@benmccann benmccann reopened this Jan 5, 2014
@hmarr
Copy link

hmarr commented Jan 5, 2014

The type assertion on line 218 still seems to cause issues when a unix socket is used.

Perhaps replace

if err := rwc.(*net.TCPConn).CloseWrite(); err != nil {
    fmt.Fprintf(errStream, "Couldn't send EOF: %s\n", err)
}

with

if err := rwc.(interface{ CloseWrite() error }).CloseWrite(); err != nil {
    fmt.Fprintf(errStream, "Couldn't send EOF: %s\n", err)
}

@benmccann
Copy link
Contributor Author

@hmarr i've done exactly that in another pull request already #52

@hmarr
Copy link

hmarr commented Jan 5, 2014

Ah, gotcha.

@andrewsmedina
Copy link
Collaborator

@benmccann thank you! Can you write a code to test it? If you need help let me know.

@benmccann
Copy link
Contributor Author

@andrewsmedina thanks for the help merging these commits! would love to get this last one in. any ideas on how to proceed? i don't really know how to write a test for this one

@benmccann
Copy link
Contributor Author

maybe this one you could merge and write a test for? the issue is that hijack does not have an existing test so i would have to write one from scratch and don't know enough go to know how to do that

@andrewsmedina
Copy link
Collaborator

@benmccann I will do it :)

@benmccann
Copy link
Contributor Author

awesome. thanks you Andrews! i will look at how you did it, so hopefully i can next time

@benmccann
Copy link
Contributor Author

btw, think we could merge this change now and then do the test in another commit?

andrewsmedina added a commit that referenced this pull request Jan 6, 2014
@andrewsmedina andrewsmedina merged commit 26ebb38 into fsouza:master Jan 6, 2014
@benmccann
Copy link
Contributor Author

awesome! thank you so much!

This pull request was closed.
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