search

gobwas / ws

Tiny WebSocket library for Go.
MIT License
6.1k stars 373 forks source link

Add Handshake.Header field #203

Open PotatoCloud opened 2 months ago

PotatoCloud commented 2 months ago

I think we should add a Header field to Handshake. Some developers may need to process the Header of the corresponding connection. At present, it is difficult for gobwas to implement similar functions.

cristaloleg commented 2 months ago

Hi, thanks for the PR. Can you clarify what you're trying to achieve?

PotatoCloud commented 2 months ago

Hi, thanks for the PR. Can you clarify what you're trying to achieve?

I copy the headers during the handshake into the Handshake struct (since currently gobwas/ws doesn't have a good way to get the handshake headers/raw headers)

PotatoCloud commented 2 months ago

Hi, thanks for the PR. Can you clarify what you're trying to achieve?

For example, some developers want to obtain the Authorization field in the header for identity authentication.

cristaloleg commented 2 months ago

I'm not 100% sure what is the point of passing request headers to handshake result. User can access exactly the same data by looking at request directly. 

hs.Header.Get("Authorization")
==
r.Header.Get("Authorization")

What is the benefit of keeping headers in a handshake?

PotatoCloud commented 2 months ago

I'm not 100% sure what is the point of passing request headers to handshake result. User can access exactly the same data by looking at request directly.

hs.Header.Get("Authorization")
==
r.Header.Get("Authorization")

What is the benefit of keeping headers in a handshake?

but, func (u Upgrader) Upgrade(conn io.ReadWriter) (hs Handshake, err error) can't do this.

For example, I want to build a pure websocket server, using net + gobwas/ws, in this case I can't do it like you do.

cristaloleg commented 2 months ago

pure websocket server

Based purely on TCP connections, right?

Ok, I think I'm starting to understand your case. The only thing that looks scary to me is that we will start allocating/copying all the headers for each request, which is completely not acceptable with the current guarantees.

The only thing I see now is to hide it under a flag.

PotatoCloud commented 2 months ago

we will start allocating/copying all the headers for each request The only thing I see now is to hide it under a flag.

i got it, i will add build tags support for it later

cristaloleg commented 2 months ago

No-no, not a build tag. Flag in upgraders.

cristaloleg commented 2 months ago

Wait a sec, have you looked at OnHeader func(key, value []byte) error ? This is exactly what you need to access all the headers without copying them (see also example in README)

PotatoCloud commented 2 months ago

OnHeader func(key, value []byte) error

I tried it and found that OnHeader captures all the incoming connection headers. So I added the headers to the Handshake

PotatoCloud commented 2 months ago

No-no, not a build tag. Flag in upgraders.

got it.

cristaloleg commented 2 months ago

all the incoming connection headers.

Exactly what you need, isn't it?

PotatoCloud commented 2 months ago

Exactly what you need, isn't it?

wrong, there is no way to know which connection the header was passed in from. for example, identity authentication needs to be performed on a specific connection, and this process cannot be completed using OnHeader

PotatoCloud commented 2 months ago

plz review it. i am not sure if what I did meets your requirements.

cristaloleg commented 2 months ago

I will take a look tomorrow, thanks.

PotatoCloud commented 2 months ago

I will take a look tomorrow, thanks.

can u merge now? thanks.