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

[FEATURE REQUEST] export more events and authentication #4

Open
roytan883 opened this issue Jul 12, 2019 · 23 comments
Open

[FEATURE REQUEST] export more events and authentication #4

roytan883 opened this issue Jul 12, 2019 · 23 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@roytan883
Copy link

Is your feature request related to a problem? Please describe.

1, I tried neffos, it works great, but seems it only focused on Message event. But for app layer, maybe want to collect some information about: connection count, each connection startTime and endTime, last activeTime, and so on.

2, send broadcast PING from server to all client. For mobile network, APP may hibernate at background, which can't send PING. so need server to send PING to remain the TCP connection and APP alive.

3, able to do authentication when websockt upgrade, like pass the http.Request and return a simple true or false. so the app layer can read url and param from http.Request. for example: http://xxxx/ws/endpoint?uid=aaa&token=bbb

Describe the solution you'd like
1, export some event for each connection, like: CONNECT, CLOSE, ACTIVE(stand for: message, ping, pong) ......

2, able to broadcast PING

3, export handler for authentication, which default return true

@roytan883 roytan883 added the enhancement New feature or request label Jul 12, 2019
@majidbigdeli
Copy link

majidbigdeli commented Jul 12, 2019

@roytan883 for authentication please see #1229

@kataras But in my opinion, some events need to be authentication, not all events

@roytan883 You have access to the total connection count with Server.GetTotalConnections()
But in my opinion, It is better to have connection count with Server.GetTotalConnectionsByNamesapce()

@roytan883 you have access to CONNECT , Close , and ... for each connection for example

`server.GetConnections()["id"].Connect()`

or
server.GetConnections()["id"].Close()

or
server.GetConnectionsByNamespace("namespace")["id"].Conn.Close()

@roytan883
Copy link
Author

roytan883 commented Jul 12, 2019

@majidbigdeli
I mean event to notify CONNECT/CLOSE/ACTIVE, not timer loop query those connections status which seems strange

1, OK, i found OnConnect and OnDisconnect form server.go now

3, #1229 seems work for auth.

2, but how to broadcast PING ?

@majidbigdeli
Copy link

majidbigdeli commented Jul 12, 2019

@roytan883 for send png to clients I use

 fileToBeUploaded := "image.png"
 file, err := os.Open(fileToBeUploaded)

  if err != nil {
          fmt.Println(err)
          os.Exit(1)
  }

  defer file.Close()

  fileInfo, _ := file.Stat()
  var size int64 = fileInfo.Size()
  bytes := make([]byte, size)

  // read file into bytes
  buffer := bufio.NewReader(file)
  _, err = buffer.Read(bytes)    // <--------------- here!

I create byte array and then send to client
and in client (javascript)
I use

<img id="ItemPreview" src="" />
document.getElementById("ItemPreview").src = "data:image/png;base64," + YourByte;

@roytan883
Copy link
Author

roytan883 commented Jul 12, 2019

@majidbigdeli Thanks for reply, but actually i'm asking websocket PING, not image PNG

I found

websocketServer.Broadcast(nil, neffos.Message{
    Namespace: "default", // <-
    Event: "toThatEvent", // <-
    Body: []byte("the data"),
    To: "X-User-1",       // <-
})

neffos.Message cant set IsNative, but can't set OpCode to PING. So seems there is no way to send websocket PING.

@majidbigdeli
Copy link

majidbigdeli commented Jul 12, 2019

@roytan883 yes .
I am Sorry . I saw the png
you can use

srv.Broadcast(nil, neffos.Message{ Event: "chat", []byte("the data")})

when Message.To is null message brodacast to all clients

@roytan883
Copy link
Author

@majidbigdeli

neffos.Message cant set IsNative, but can not set OpCode PING. So seems there is no way to send websocket layer PING.

@majidbigdeli
Copy link

@majidbigdeli

neffos.Message cant set IsNative, but can not set OpCode PING. So seems there is no way to send websocket layer PING.

yes. it is true

@roytan883
Copy link
Author

roytan883 commented Jul 12, 2019

@majidbigdeli
actually, I'm trying to implement two phases timeout on server.

For example: total readTimeout-> 210s + 30s

1, set normal connection readTimeout is 210s
2, if in 210s, connection not received any MESSAGE/PING/PONG, server send a PING on this connection, and reset readTimeout to 30s
3, if in 30s, still not receive any MESSAGE/PING/PONG, server close this connection
4, if in 210s + 30s, received any MESSAGE/PING/PONG, reset readTimeout to 210s

Normally the client should send PING interval few minutes.
Add this strategy can somehow solve the mobile network APP background hibernate can not receive message issue.

So may need Conn struct add those methods:

  • OnReadTimeout (which not close the connectioin)
  • SendPing
  • GetReadTimeout / SetReadTimeout

@majidbigdeli
Copy link

@kataras @roytan883

neffos have SetTimeouts func for example https://github.com/kataras/neffos/blob/master/_examples/struct-handler/main.go
for example if a client or server didn't receive or sent something for 20 seconds this connection will be terminated.
@kataras But this have not two phases timeout on server.
it is good feature

	controller := new(serverConn)
	controller.SuffixResponse = "Static Response Suffix for shake of the example"

	// This will convert a structure to neffos.Namespaces based on the struct's methods.
	// The methods can be func(msg neffos.Message) error if the structure contains a *neffos.NSConn field,
	// otherwise they should be like any event callback: func(nsConn *neffos.NSConn, msg neffos.Message) error.
	// If contains a field of type *neffos.NSConn then a new controller
	// is created on each new connection to this namespace
	// and static fields(if any) are set on runtime with the NSConn itself.
	// If it's a static controller (does not contain a NSConn field)
	// then it just registers its functions as regular events without performance cost.
	events := neffos.NewStruct(controller).
		// Optionally, sets read and write deadlines on the underlying network connection.
		// After a read or write have timed out, the websocket connection is closed.
		// For example:
		// If a client or server didn't receive or sent something
		// for 20 seconds this connection will be terminated.
		SetTimeouts(20*time.Second, 20*time.Second).
		// This will convert the "OnChat" method to a "Chat" event instead.
		SetEventMatcher(neffos.EventTrimPrefixMatcher("On"))

	websocketServer := neffos.New(gobwas.DefaultUpgrader, events)

@majidbigdeli
Copy link

@majidbigdeli
actually, I'm trying to implement two phases timeout on server.

For example: total readTimeout-> 210s + 30s

1, set normal connection readTimeout is 210s
2, if in 210s, connection not received any MESSAGE/PING/PONG, server send a PING on this connection, and reset readTimeout to 30s
3, if in 30s, still not receive any MESSAGE/PING/PONG, server close this connection
4, if in 210s + 30s, received any MESSAGE/PING/PONG, reset readTimeout to 210s

Normally the client should send PING interval few minutes.
Add this strategy can somehow solve the mobile network APP background hibernate can not receive message issue.

So may need Conn struct add those methods:

  • OnReadTimeout (which not close the connectioin)
  • SendPing
  • GetReadTimeout / SetReadTimeout

it is good feature

@roytan883
Copy link
Author

if this function not close the socket, seems can be add here. Maybe I can send a PR for this.

func (s *Socket) ReadData(timeout time.Duration) ([]byte, error) {
	for {
		if timeout > 0 {
			s.UnderlyingConn.SetReadDeadline(time.Now().Add(timeout))
		}

		opCode, data, err := s.UnderlyingConn.ReadMessage()
		if err != nil {
			return nil, err
		}

		if opCode != gorilla.BinaryMessage && opCode != gorilla.TextMessage {
			// if gorilla.IsUnexpectedCloseError(err, gorilla.CloseGoingAway) ...
			continue
		}

		return data, err
	}
}

@kataras
Copy link
Owner

kataras commented Jul 12, 2019

Hello @roytan883, as I see you, with some help of @majidbigdeli, found the answers in all of your requests/questions except the broadcast websocket PING special message and set timeouts on runtime.

Please answer my questions below so I can introduce a feature that will be helpful for you and others.

@roytan883

So may need Conn struct add those methods: OnReadTimeout (which not close the connection SendPing GetReadTimeout / SetReadTimeout

You can already set them with neffos.WithTimeout or through upgrader/dialer underline implementation.

  1. So you want to change the timeouts on runtime, right?

You pointed out correctly the Socket, by-default it does send and read only binary/text messages to the Conn.handleMessage but this doesn't mean that the PING,PONG messages are blocked, they are still respected but we just don't handle them. You may noticed thatthat you can override or wrap an existing socket, so you can implement your own logic there without the need to modify the internal neffos source code. However, I want to be more helpful, so

  1. in short you want events[OnPong] = neffos.MessageHandlerFunc to catch ping replies and Conn.Ping() to send ping method?

Also, @majidbigdeli, about:

@roytan883 you have access to CONNECT , Close , and ... for each connection for example server.GetConnections()["id"].Connect() or server.GetConnections()["id"].Close() or server.GetConnectionsByNamespace("namespace")["id"].Conn.Close()

These functions these are designed to give statistics and not to handle common operations, each time you call one of those the whole server blocks and can't accept a new connection(except the GetTotalConnections which does not block), this is why we have special functions for broadcasting, catch on connect event, close all connections and etc. Use server.OnConnect, OnDisconnect instead.

Thanks

@kataras kataras added the good first issue Good for newcomers label Jul 12, 2019
@roytan883
Copy link
Author

roytan883 commented Jul 12, 2019

@kataras after try to implement two-phases-timeout by modify gobwas/gorilla's socket.go this afternoon (it works), I think it maybe called keepalive is much better.

override or wrap an existing socket

is a good idea, also i can implement ACTIVE event here sending to my app layer.

@kataras
Copy link
Owner

kataras commented Jul 12, 2019

Yes, exactly, so you see how modular is neffos, you can create new or wrap an existing compatible upgrader/dialer. For example on kataras/iris, to keep the iris.Context linked with the Socket I used a socket wrapper which does not require full wrapping of existing upgrader, just the Socket one.

@kataras
Copy link
Owner

kataras commented Jul 12, 2019

You can push a PR when you done with this @roytan883, just note that if your changes have any performance cost create a new file, named wrapper or anything you want and push them on their directories neffos/gobwas and neffos/gorilla in order to be optional to used in top of their original implementations. Keep me updated to help you out with designing! Thanks for using neffos man

@roytan883
Copy link
Author

roytan883 commented Jul 13, 2019

@kataras I send a PR #6 implement keepalive by create a new upgrader/dialer called gobwasalive, please take a look

@roytan883
Copy link
Author

@kataras BTW, I found that MUST reset WriteDeadline to zero after write any thing, otherwise the socket will receive a EOF error. It maybe a bug also need to be fix in gobwas/gorilla socket.go:

func (s *Socket) write(body []byte, op gobwas.OpCode, timeout time.Duration) error {
    s.mu.Lock()
    if timeout > 0 {
        if err := s.UnderlyingConn.SetWriteDeadline(time.Now().Add(timeout)); err != nil {
            log.Fatalf("gobwasalive Socket SetWriteDeadline, err: %v", err)
        }
    }

    // println("write: " + string(body))
    err := wsutil.WriteMessage(s.UnderlyingConn, s.state, op, body)

    if timeout > 0 { //must set it back to zero, otherwise it will get EOF error
        if err := s.UnderlyingConn.SetWriteDeadline(time.Time{}); err != nil {
            log.Fatalf("gobwasalive Socket SetWriteDeadline back, err: %v", err)
        }
    }

    s.mu.Unlock()

    return err
}

@kataras
Copy link
Owner

kataras commented Jul 13, 2019

@roytan883 So you made a total new subpacke for var twServer/twDialer = timingwheel.NewTimingWheel(time.Millisecond, 20) and use it inside Socket. I saw the PR and it does contain a lot of duplications of existing neffos code, I will push changes later on so you can see how u can adapt that to existings upgraders and dialer, tell me when its ready. Thanks a kot.

@kataras
Copy link
Owner

kataras commented Jul 13, 2019

About deadline, gorilla, ehich has s setting for it, does not reset it to 0 when write is done, also I dont think itmust reset to 0 because on the next call it will reset it self to the value(except if te value changes to 0, sre you refering to that case?). Setting twice the deadline in the same write/read op is not acceptable, there is another solution to clear the deadline: check if deadline value was >0 and if current value is 0 and then set it to 0 else set, on beginning once not in the end

@roytan883
Copy link
Author

roytan883 commented Jul 13, 2019

@kataras you can try my PR, in the example code, if not reset the deadline to zero, it will receive EOF at neffos.WithTimeout{ WriteTimeout: 20 * time.Second.

It looks strange, why writeDeadline conflict with ReadData. I debug it, each PING sending has no error, but it always EOF after 20s. every PING write has 3s timeout at beginning, but it's not helpful.

after add those code:

if timeout > 0 { //must set it back to zero, otherwise it will get `EOF` error
        if err := s.UnderlyingConn.SetWriteDeadline(time.Time{}); err != nil {
            log.Fatalf("gobwasalive Socket SetWriteDeadline back, err: %v", err)
        }
    }

socket never got EOF.

I test 1 server with 10000 client, works great. But the server take 330MB memory on WINDOWS 10, is it too high or normal ?

@kataras
Copy link
Owner

kataras commented Jul 13, 2019

Yes, if you run it from _examples/stress-test which broadcasts to all clients and sends-receives messages and keep some connections alive, for 10k it's 234MB here and it's absolutely fine, more than fine. However, I just updated the go.mod to gobwas/ws 1.0.2 as well, it contains a good fix that you will need if u use go 1.12.6+

About the PR, tell me when you test every case, and complete your changes and etc. and I will push my own changes to your PR so we can work together, because it doesn't need to live on other subpackage with code duplications from existing packages, and for the deadlines, you mean you never got EOF so the

return ioutil.ReadAll(s.reader)
never returns? But it returns (it returns when EOF), please give me more information about this because I can't reproduce it, is it because of the "alive" feature you are trying to add or it's a current thing? Is it only on gobwas or the same behavior exist on gorilla too?

Thanks @roytan883

@roytan883
Copy link
Author

roytan883 commented Jul 13, 2019

@kataras

1, for this PR, only about keepalive feature. what test case should i write? I create it by a new gobwasalive is trying to not impact old code as i'm not familiar with neffos structure right now. And i already add a example at _examples/gobwasalive/main.go. yes, i think some part of it is silly, like the idleTime by NewDialer/NewUpgrader and the global timingwheel. I have not wrote any GO code few months, some part of GO is hazy for me.

But i think the idea about Keepalive is good, i'm happy that you modify this PR or just implement yours which may much better than this one. Most I care is that server will send PING before timeout, it is very important at mobile network to keep my APP socket alive.

2, neffos/gobwas/socket.go return ioutil.ReadAll(s.reader) works fine. I say never got EOF is just about the writing PING and receive EOF error. Normal read return EOF still work (which i use CTRL+C to close client).

3, for deadline, yesterday when i implement two-phases-timeout by modify the gobwas/gorilla socket.go, both got EOF after PING. gorilla is worse, it got EOF right after send PING; gobwas can send a few PING before EOF at neffos.WithTimeout{ WriteTimeout: 20 * time.Second. I guest both related to the writeDeadline reset issue.

reproduce it is simple, just disable

if timeout > 0 { //must set it back to zero, otherwise it will get `EOF` error
        if err := s.UnderlyingConn.SetWriteDeadline(time.Time{}); err != nil {
            log.Fatalf("gobwasalive Socket SetWriteDeadline back, err: %v", err)
        }
    }

and add some Printf in gobwasalive/socket.go ReadData, then run _examples/gobwasalive/main.go. you will find that client will disconnect at 20s by EOF.

@roytan883
Copy link
Author

roytan883 commented Jul 15, 2019

after optimize timingwheel to:

timingwheel.NewTimingWheel(50*time.Millisecond, 100)

CPU and memory looks good now, 10K clients use 240MB on server, and CPU nearly 0%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants