nats-io / nats.go

Golang client for NATS, the cloud native messaging system.
https://nats.io
Apache License 2.0
5.52k stars 696 forks source link

Subject of empty string will cause the nats connection closed #62

Closed guyj93 closed 9 years ago

guyj93 commented 9 years ago

Hello, I found that if I give an empty string to the subject parameter of "Publish","Request","Subscribe" function, the function will return no error. However, operations like these will cause an ERR in gnatsd, and gnatsd will close the connection. After that, calling any function of the connection will return an error of "nats: Connection Closed", and the client won't do any reconnection attempt. The test code following will cause this condition.

// testNats project main.go
package main

import (
    "fmt"
    "time"

    "github.com/nats-io/nats"
)

func main() {
    nc, err := nats.Connect(nats.DefaultURL)
    if err != nil {
        panic(err)
    }
    defer nc.Close()
    //subscribe 'foo'
    handle := func(m *nats.Msg) {
        fmt.Println(string(m.Data))
    }
    if _, err := nc.Subscribe("foo", handle); err != nil {
        fmt.Println(err)
    }

    //these are invalid operation
    if _, err := nc.Subscribe("", handle); err != nil {
        fmt.Println(err)
    }
    if err := nc.Publish("", nil); err != nil {
        fmt.Println(err)
    }

    //wait for client to send invalid operation to gnatsd
    time.Sleep(time.Second)

    //publish something
    if err := nc.Publish("foo", []byte("bar")); err != nil {
        fmt.Println(err)
    }
    time.Sleep(time.Second)
}

Result:

C:/myGO/src/testNats/testNats.exe  [C:/myGO/src/testNats]
nats: Connection Closed
Success: process exited with code 0.

I think that the private function "subscribe" and "publish" should return an error if we give an empty string subject, but I found no checkout for the subject parameter in these function.

The dangerous condition may easily happened in this way:

// Requests
msg, err := nc.Request("help", []byte("help me"), 10*time.Millisecond)

// Replies
nc.Subscribe("help", func(m *Msg) {
    nc.Publish(m.Reply, []byte("I can help!"))
})

//invalid Request
//nc.Publish("help", []byte("help me"))

If other Programs use 'Publish' instead of 'Request' to publish something to the subject 'help', then m.Reply == "" .

Sorry for my poor English. ^_^

derekcollison commented 9 years ago

Currently the best way to handle this is to utilize an asyncErrorCB.

https://github.com/nats-io/nats/blob/master/nats.go#L104

You can see an example here..

https://github.com/nats-io/nats/blob/d19a6cec4083353ca586e7052a89e0f719946ee5/sub_test.go#L165

The client should be default reconnect on a disconnect.

However, I did add a TODO item to look at this more closely.

https://github.com/nats-io/nats/blob/master/TODO.md

=derek

On Fri, Aug 21, 2015 at 1:25 AM, guyj93 notifications@github.com wrote:

Hello, I found that if I give an empty string to the subject parameter of "Publish","Request","Subscribe" function, the function will return no error. However, operations like these will cause an ERR in gnatsd, and gnatsd will close the connection. After that call any function of the connection will return an error of "nats: Connection Closed", and the client won't do any reconnection attempt. The test code following will cause this condition.

// testNats project main.gopackage main import ( "fmt" "time"

"github.com/nats-io/nats"

) func main() { nc, err := nats.Connect(nats.DefaultURL) if err != nil { panic(err) } defer nc.Close() //subscribe 'foo' handle := func(m *nats.Msg) { fmt.Println(string(m.Data)) } if _, err := nc.Subscribe("foo", handle); err != nil { fmt.Println(err) }

//these are invalid operation
if _, err := nc.Subscribe("", handle); err != nil {
    fmt.Println(err)
}
if err := nc.Publish("", nil); err != nil {
    fmt.Println(err)
}

//wait for client to send invalid operation to gnatsd
time.Sleep(time.Second)

//publish something
if err := nc.Publish("foo", []byte("bar")); err != nil {
    fmt.Println(err)
}
time.Sleep(time.Second)

}

Result:

C:/myGO/src/testNats/testNats.exe [C:/myGO/src/testNats] nats: Connection Closed Success: process exited with code 0.

I think that the private function "subscribe" and "publish" should return an error if we give an empty string subject, but I found no checkout for the subject parameter in these function.

The dangerous condition may easily happened in this way:

// Requestsmsg, err := nc.Request("help", []byte("help me"), 10time.Millisecond) // Replies nc.Subscribe("help", func(m Msg) { nc.Publish(m.Reply, []byte("I can help!")) })

If other Programs use Publish instead of Request to publish something to subject 'help', then m.Reply == "" .

Sorry for my poor English. ^_^

— Reply to this email directly or view it on GitHub https://github.com/nats-io/nats/issues/62.

guyj93 commented 9 years ago

Thank you for your reply !

But I find that asyncErrorCB can't help me in this issue. In the following code, asyncErrorCB won't been triggered.

package main

import (
    "fmt"
    "time"

    "github.com/nats-io/nats"
)

func main() {
    opt := nats.DefaultOptions
    opt.Servers = []string{nats.DefaultURL}
    opt.AsyncErrorCB = func(c *nats.Conn, s *nats.Subscription, e error) {
        fmt.Printf("%v: AsyncErrorCB: %v\n", time.Now(), e)
    }
    nc, err := opt.Connect()
    if err != nil {
        panic(err)
    }
    if err = nc.Publish("", nil); err != nil {
        fmt.Printf("%v: main: %v\n", time.Now(), err)
    } else {
        fmt.Printf("%v: main: Succ to Pub\n", time.Now())
    }
    //if I use Flush func, the func will return an error
    //  if err = nc.Flush(); err != nil {
    //      fmt.Printf("%v: main: %v\n", time.Now(), err)
    //  }
    time.Sleep(time.Second)
    if err = nc.Publish("foo", nil); err != nil {
        fmt.Printf("%v: main: %v\n", time.Now(), err)
    }
}

Result:

2015-08-22 13:44:31.7658178 +0800 CST: main: Succ to Pub
2015-08-22 13:44:32.766875 +0800 CST: main: nats: Connection Closed

If I use nc.Flush() to flush out the publish, the result will be:

2015-08-22 13:45:02.4485727 +0800 CST: main: Succ to Pub
2015-08-22 13:45:02.4505729 +0800 CST: main: nats: 'Parser Error'
2015-08-22 13:45:03.4506301 +0800 CST: main: nats: Connection Closed

I found the comments on the definition of ErrHandler, and it seems that the handler can only be triggered while receving something.

// ErrHandlers are used to process asynchronous errors encountered
// while processing inbound messages.
type ErrHandler func(*Conn, *Subscription, error)

In our group to handle this issue, we directly modified the private function "publish" and "subscribe" in 'nats.go' and let them return an error when receving empty string as the 'subj' parameter.

derekcollison commented 9 years ago

The server should return an error protocol message to the client. Which server are you using? gnatsd? Thanks again, will dig into it for sure since the async handler should be firing.

On Fri, Aug 21, 2015 at 10:40 PM, guyj93 notifications@github.com wrote:

Thank you for your reply !

But I find that asyncErrorCB can't help me in this issue. In the following code, asyncErrorCB won't been triggered.

package main import ( "fmt" "time"

"github.com/nats-io/nats"

) func main() { opt := nats.DefaultOptions opt.Servers = []string{nats.DefaultURL} opt.AsyncErrorCB = func(c nats.Conn, s nats.Subscription, e error) { fmt.Printf("%v: AsyncErrorCB: %v\n", time.Now(), e) } nc, err := opt.Connect() if err != nil { panic(err) } if err = nc.Publish("", nil); err != nil { fmt.Printf("%v: main: %v\n", time.Now(), err) } else { fmt.Printf("%v: main: Succ to Pub\n", time.Now()) } //if I use Flush func, the func will return an error // if err = nc.Flush(); err != nil { // fmt.Printf("%v: main: %v\n", time.Now(), err) // } time.Sleep(time.Second) if err = nc.Publish("foo", nil); err != nil { fmt.Printf("%v: main: %v\n", time.Now(), err) } }

Result:

2015-08-22 13:19:13.1489579 +0800 CST: main: Succ to Pub 2015-08-22 13:19:14.1500151 +0800 CST: main: nats: Connection Closed

If I use nc.Flush() to flush out the publish, the result will be:

2015-08-22 13:31:27.5719645 +0800 CST: main: Succ to Pub 2015-08-22 13:31:27.5739646 +0800 CST: main: nats: 'Parser Error' 2015-08-22 13:31:28.5740218 +0800 CST: main: nats: Connection Closed

I found the comments on the definition of ErrHandler, and it seems that the handler can only be triggered while receving something.

// ErrHandlers are used to process asynchronous errors encountered// while processing inbound messages.type ErrHandler func(Conn, Subscription, error)

In our group to handle this issue, we directly modified the private function "publish" and "subscribe" in 'nats.go' and let them return an error when receving empty string as the 'subj' parameter.

— Reply to this email directly or view it on GitHub https://github.com/nats-io/nats/issues/62#issuecomment-133637956.

guyj93 commented 9 years ago

I use gnatsd as my server. Here is the logs of gnatsd

C:\>gnatsd -V
[9372] 2015/08/22 14:16:28.348440 [INF] Starting gnatsd version 0.6.1.beta
[9372] 2015/08/22 14:16:28.349440 [INF] Listening for client connections on 0.0.
0.0:4222
[9372] 2015/08/22 14:16:28.349440 [INF] gnatsd is ready
[9372] 2015/08/22 14:16:35.242834 [TRC] 127.0.0.1:6225 - cid:1 - ->> [CONNECT {"
verbose":false,"pedantic":false,"ssl_required":false,"name":"","lang":"go","vers
ion":"1.0.9"}]
[9372] 2015/08/22 14:16:35.242834 [TRC] 127.0.0.1:6225 - cid:1 - ->> [PING]
[9372] 2015/08/22 14:16:35.242834 [TRC] 127.0.0.1:6225 - cid:1 - <<- [PONG]
[9372] 2015/08/22 14:16:35.244834 [TRC] 127.0.0.1:6225 - cid:1 - ->> [PUB 0]
[9372] 2015/08/22 14:16:35.244834 [ERR] 127.0.0.1:6225 - cid:1 - Error reading f
rom client: processPub Parse Error: '0'
guyj93 commented 9 years ago

I think client has already receive the error protocol message from server, otherwise the function nc.Flush() won't return an error of " nats: 'Parser Error' ".

derekcollison commented 9 years ago

Thanks, this is helpful. Appreciate the feedback.