cloudwebrtc / go-protoo

A minimalist and extensible go signaling framework for WebRTC.
MIT License
23 stars 6 forks source link

support status code for closing websocket #16

Closed brucekim closed 3 years ago

brucekim commented 3 years ago

Current WebSocketTransport.close() only returns code number '100' regardless of any specific reason of websocket closed.

Why not to support correct status code in order for the module relying on go-protoo package to properly handle error situation happened to websocket?

RFC6455 specifies several websocket close codes. I think it would be better to comply with it for better project. I checked 'gollira/websocket' also supports/return websocket close code as specified in RFC6455.

In addition, one of project that relies on go-protoo has a defect due to the lack of status code for websocket close as below

It would be enough to keep status code with description delivered by gollira/websocket and send it through transport.onClose channel.

Please review my feedback.

` handleClose := func(code int, err string) { if allowClientDisconnect { log.Infof("signal.in handleClose.AllowDisconnected => peer (%s)", peer.ID()) return }

    roomLock.RLock()
    defer roomLock.RUnlock()
    rooms := GetRoomsByPeer(peer.ID())
    log.Infof("signal.in handleClose [%d] %s rooms=%v", code, err, rooms)
    for _, room := range rooms {
        if room != nil {
            oldPeer := room.GetPeer(peer.ID())
            // only remove if its the same peer. If newer peer joined before the cleanup, leave it.
            if oldPeer == &peer.Peer {
                if code > 1000 {
                    msg := proto.LeaveMsg{
                        RoomInfo: proto.RoomInfo{RID: room.ID()},
                    }
                    msgStr, _ := json.Marshal(msg)
                    bizCall(proto.ClientLeave, peer, msgStr, connectionClaims, emptyAccept, reject)
                }
                log.Infof("signal.in handleClose removing peer (%s) from room (%s)", peer.ID(), room.ID())
                room.RemovePeer(peer.ID())
            }
        }
    }
    log.Infof("signal.in handleClose => peer (%s) ", peer.ID())
}

`

Is there any reason to use code number '100' returning in close()?

 |Status Code | Meaning         | Contact       | Reference |
-+------------+-----------------+---------------+-----------|
 | 1000       | Normal Closure  | hybi@ietf.org | RFC 6455  |
-+------------+-----------------+---------------+-----------|
 | 1001       | Going Away      | hybi@ietf.org | RFC 6455  |
-+------------+-----------------+---------------+-----------|
 | 1002       | Protocol error  | hybi@ietf.org | RFC 6455  |
-+------------+-----------------+---------------+-----------|
 | 1003       | Unsupported Data| hybi@ietf.org | RFC 6455  |
-+------------+-----------------+---------------+-----------|
 | 1004       | ---Reserved---- | hybi@ietf.org | RFC 6455  |
-+------------+-----------------+---------------+-----------|
 | 1005       | No Status Rcvd  | hybi@ietf.org | RFC 6455  |
-+------------+-----------------+---------------+-----------|
 | 1006       | Abnormal Closure| hybi@ietf.org | RFC 6455  |
-+------------+-----------------+---------------+-----------|
 | 1007       | Invalid frame   | hybi@ietf.org | RFC 6455  |
 |            | payload data    |               |           |
-+------------+-----------------+---------------+-----------|
 | 1008       | Policy Violation| hybi@ietf.org | RFC 6455  |
-+------------+-----------------+---------------+-----------|
 | 1009       | Message Too Big | hybi@ietf.org | RFC 6455  |
-+------------+-----------------+---------------+-----------|
 | 1010       | Mandatory Ext.  | hybi@ietf.org | RFC 6455  |
-+------------+-----------------+---------------+-----------|
 | 1011       | Internal Server | hybi@ietf.org | RFC 6455  |
 |            | Error           |               |           |
-+------------+-----------------+---------------+-----------|
 | 1015       | TLS handshake   | hybi@ietf.org | RFC 6455  |
-+------------+-----------------+---------------+-----------|