stompngo should not panic when parsing headers, return an error instead. about stompngo HOT 12 CLOSED

Hmmmm ...

Bounds checking and a custom error might not be a bad idea here. If you have the interest please proceed with work on it. We can discuss it as and when required.

However ..... I need to ask a different question: what the heck are you getting back from Apollo that triggers this???

Lots of details that make me ask. I test with Apollo on a regular basis.

What is in 'f.Command' at that point?

It is clearly not a 1.0 connection. Are heartbeats enabled? If so, what are the negotiated heartbeat intervals?

Thanks, Guy

from stompngo.

Sorry, I really want to know what is in 'p' at that point. 'f.Command' would be informative, but 'p' much more so .....

from stompngo.

@gmallard I've been trying to repo this for about an hour and no luck. Unfortunately I flushed the queue and things started working again.

I've been scratching my head regrading how it could be possible for the Broker to send back an invalid frame. The reason why I think it's the broker is that I got that error from two different hosts and repeatedly after restarting the application. This kinda rules out a tcp connection issue, but I can't be sure.

from stompngo.

@gmallard To answer some of the questions:

I'm running a stock apache apollo 1.6 on openjdk 1.6 on ubuntu in EC2.

The section of code that triggered the error:

    log.Println("Connecting to stomp broker:", c.host)
    conn, err := net.Dial("tcp", c.host)
    if err != nil {
        log.Fatal(err.Error())
    }
    headers := stompngo.Headers{
        "accept-version", "1.1",
        "login", c.user,
        "passcode", c.pass,
    }
    stomp, err := stompngo.Connect(conn, headers)
    if err != nil {
        log.Fatal(err.Error())
    }

    id := stompngo.Uuid()
    subscribeHeaders := stompngo.Headers{
        "ack", "client-individual",
        "destination", c.dest,
        "id", id,
    }
    subscription, err := stomp.Subscribe(subscribeHeaders)
    if err != nil {
        log.Fatal(err.Error())
    }

The above is running within a goroutine started in main.

from stompngo.

@kelseyhightower I see absolutely nothing unusual about that code. Or that it might be running in a goroutine.

The inability to reproduce is most unfortunate.

I will ponder this for a bit, and get back to you if I gain any insight.

from stompngo.

For the record:

I tried several ways to recreate this with this package and Apollo. Experiments:

• Empty header value
• Empty header key
• Both key and value empty

No joy from any of any of those experiments. All work as I expected (using ActiveMQ is a different story though).

The only way I can make strings.SplitN return a 1 element slice is when the original string to split is already empty. Ref:

http://play.golang.org/p/UO2ZZ_F4CF

That leads me to very odd conclusions about the inbound data you saw coming in to readFrame().

I am thinking there ought to be two sanity checks in readFrame():

• once f.Command is assigned, check that it is one of: MESSAGE, ERROR, or RECEIPT
• check that the length of the slice after SplitN == 2

If either test fails, create a reasonable error, and return that to the caller (c.reader()).

When c.reader() sees a non-nil error it attempts to push a MessageData struct with the error out to any and all subscribers. I am now considering that it should perhaps also attempt to forcibly close the connection (call c.shutdown()).

If you are interested in pursuing work on any of this, please proceed.

G.

from stompngo.

A side comment: I think Apollo and stompngo err when allowing empty header names. The way I read the specs' BNF, that should not be allowed.

I want to wait and see what Hiram says about Apollo. Ref:

https://issues.apache.org/jira/browse/APLO-349

from stompngo.

@gmallard Thanks for researching this so heavily, I'll be working on a PR and you should see it come across later today, or early tomorrow.

from stompngo.

I'll be working on another PR to handle the check for invalid commands received by the broker.

from stompngo.

Very good. Let's leave this issue open for now.

from stompngo.

Closing, per:

4459a22 Validate commands from broker.

from stompngo.

If you are still using the stompngo package please try the current v.1.0.2 tag.

Tag information:

b1f1d96 refs/tags/v1.0.2

Document that version number in any issue reports.

Thanks for the support.

Guy

from stompngo.