search

timehop / apns

A Go package to interface with the Apple Push Notification Service
https://godoc.org/github.com/timehop/apns
MIT License
185 stars 47 forks source link

Report Error for Malformed Device Token #4

Closed taylortrimble closed 1 year ago

taylortrimble commented 10 years ago

I just spent a long time tracking down a bug where I was passing a byte slice for the device identifier instead of a hex encoded string. I took an awfully long time, since I didn't have an error come out of Client.FailedNotifs.

We should do that. :blush:

bdotdub commented 10 years ago

Yikes! Sorry about that. I'll have a look at it – it may take a few days since I'm currently on my honeymoon :smile:

taylortrimble commented 10 years ago

Wow! Congratulations! :wedding:

Don't even think about fixing it on your honeymoon! I want to see gray squares on your GH profile every day you're gone! :wink:

nathany commented 9 years ago

@bdotdub Congrats.

I hope somebody at timehop can look at the pull requests I've been sending. I'm vendoring a copy with godep, but it still would be nice to push things upstream (and get some feedback if I'm doing it wrong).

bdotdub commented 9 years ago

@nathany Hey Nathan – sorry about that! I'll take a look at them soon. My github notifs are a mess so they got lost in the middle of everything else.

nathany commented 9 years ago

@tylrtrmbl Personally I'm validating the tokens before sending them to timehop APNS.

func isDeviceTokenValid(s string) bool {
    if len(s) != 64 {
        return false
    }
    _, err := hex.DecodeString(s)
    return err == nil
}

The reason you didn't see an error is because it's currently ignoring errors from ToBinary. See client.go:180.

What I think should happen, is to move ToBinary sooner in the process so that Send() can return an error.

bdotdub commented 9 years ago

@nathany I'd buy that. Instead of having Client read off of a channel of Notification, it should read a channel of []byte. It'll isolate the possible errors to _, err = c.Conn.Write(b)

nathany commented 9 years ago

Sounds good.

I may be able to take a stab at it, but I'd like to resolve the current open pull requests first. Thanks!

taylortrimble commented 9 years ago

I think it is a lot clearer what's going on if you have a channel of Notification; what I'd recommend instead is removing the logging you have now in the run loop, and replacing it with an error channel on the client.

That way I can handle the error any way I see fit; log it to Sentry, for example.

taylortrimble commented 9 years ago

Ignore my last message, I haven't seen the code in a while and I think I was wrong...