- Notifications
You must be signed in to change notification settings - Fork329
Returns real remote and local address instead mocked#299
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ZackaryWelch commentedMay 5, 2022
I would like to get this in if possible. It's the only thing this library lacks from gorilla/websocket. |
netconn_notjs.go Outdated
| } | ||
| func (c *netConn) RemoteAddr() net.Addr { | ||
| if ra, ok := c.c.rwc.(LocalRemoteAddr); ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure how this could work? Have you tested this?c.c.rwc is not anet.Conn and so doesn't have these methods you're checking for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It only works in tests because the tests don't use a real http transport/server, they're using my pipe abstraction. Seehttps://github.com/nhooyr/websocket/blob/master/internal/test/wstest/pipe.go
photostormMar 8, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It works fine. Its check the reader/writer if it also has RemoteAddr() and LocalAddr() defined. It is not checking if it net.conn.
photostormMar 8, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@nhooyr Here is example of something similar to what I did:
https://github.com/golang/go/blob/master/src/io/io.go#L408
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Have you tested this in production? There's no RemoteAddr or LocalAddr on the bufio.ReadWriter returned to us from the transport.
photostormMar 8, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This the echo example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok now I'm super confused I'll have to look into how that's possible lol
Going to close as I'm assuming this approach won't work. Leaving the issue open at#327 though so I can figure something out here. |
Ah I understand, it works server side but would not work with connections from |
What is being store with io.ReadWriterClose when using the websocket.Dial? io.ReadWriterClose is just common abstraction. If the io.ReadWriterClose is being used with net.Conn then we know the local address and remote address methods exists. Just because we see io.ReadWriterClose when looking at the code does not hide the ability to access other methods if what being stored there during runtime supports these methods. Can you provide any screenshots of what is being stored there? |
I don't fully remember the type but it's not a net.Conn. Grep the net/http.Transport code for protocol upgrade and you'll find it. I'm heading out right now otherwise I'd link you. |
I will do exploring with that when I get some time. Thanks for your responses. |
Seehttps://github.com/nhooyr/websocket/blob/14fb98eba64eeb5e9d06a88b98c47ae924ac82b4/dial.go#L128 We don't get a |
Thanks@photostorm |
Currently, RemoteAddr() and LocalAddr() are mocked. This change allow the real RemoteAddr() and LocalAddr() to be used.