bradleyfalzon / gopherci

GopherCI was a project to help you maintain high-quality Go projects, by checking each GitHub Pull Request, for backward incompatible changes, and a suite of other third party static analysis tools.
https://gopherci.io
BSD 2-Clause "Simplified" License
102 stars 13 forks source link

Panic in FilterIssues with a coment without a position #92

Closed fortytw2 closed 7 years ago

fortytw2 commented 7 years ago

Not entirely sure what happened here, but the Github status check seems to be stuck in yellow

screen shot 2017-05-01 at 8 21 12 pm

PR - https://github.com/fortytw2/hydrocarbon/pull/51 GopherCI - https://gci.gopherci.io/analysis/329

bradleyfalzon commented 7 years ago

Thanks @fortytw2, the issue is with a panic on line https://github.com/bradleyfalzon/gopherci/blob/bafe2bb3318e34e0b92d39514b219555b942bb4a/internal/github/installation.go#L124

May 02 06:42:06 www1 gopherci[10430]: panic: runtime error: invalid memory address or nil pointer dereference
May 02 06:42:06 www1 gopherci[10430]: [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x80f4f2]
May 02 06:42:06 www1 gopherci[10430]: goroutine 23306 [running]:
May 02 06:42:06 www1 gopherci[10430]: github.com/bradleyfalzon/gopherci/internal/github.(*Installation).FilterIssues(0xc420362850, 0x11326a0, 0xc4202eba40, 0xc420341000, 0x8, 0xc420341044, 0xb, 0x33, 0xc4205d5140, 0x1, ...)
May 02 06:42:06 www1 gopherci[10430]: /data/gopherci-2017-04-27-141809/src/github.com/bradleyfalzon/gopherci/internal/github/installation.go:124 +0x232
May 02 06:42:06 www1 gopherci[10430]: github.com/bradleyfalzon/gopherci/internal/github.(*GitHub).Analyse(0xc4201b8480, 0x1, 0x18e2, 0x37b35a9, 0xbc61d1, 0xe, 0xc42075d7a0, 0x63, 0x0, 0x0, ...)
May 02 06:42:06 www1 gopherci[10430]: /data/gopherci-2017-04-27-141809/src/github.com/bradleyfalzon/gopherci/internal/github/handlers.go:382 +0xb23
May 02 06:42:06 www1 gopherci[10430]: main.(*queueProcessor).Process(0xc42018c108, 0xace060, 0xc420619380)
May 02 06:42:06 www1 gopherci[10430]: /data/gopherci-2017-04-27-141809/src/github.com/bradleyfalzon/gopherci/main.go:212 +0x585
May 02 06:42:06 www1 gopherci[10430]: main.(*queueProcessor).Process-fm(0xace060, 0xc420619380)
May 02 06:42:06 www1 gopherci[10430]: /data/gopherci-2017-04-27-141809/src/github.com/bradleyfalzon/gopherci/main.go:152 +0x3e
May 02 06:42:06 www1 gopherci[10430]: github.com/bradleyfalzon/gopherci/internal/queue.(*GCPPubSubQueue).receive.func1(0x7fe427eee2c8, 0xc42043c640, 0xc420409c00)
May 02 06:42:06 www1 gopherci[10430]: /data/gopherci-2017-04-27-141809/src/github.com/bradleyfalzon/gopherci/internal/queue/gcp-pubsub.go:174 +0x5b1
May 02 06:42:06 www1 gopherci[10430]: cloud.google.com/go/pubsub.(*Subscription).receive.func2(0xc4203a7770, 0xc420183c80, 0xc420409c00, 0xc4203bcc70, 0x7fe427eee2c8, 0xc42043c640)
May 02 06:42:06 www1 gopherci[10430]: /data/gopherci-2017-04-27-141809/src/cloud.google.com/go/pubsub/subscription.go:286 +0xa2
May 02 06:42:06 www1 gopherci[10430]: created by cloud.google.com/go/pubsub.(*Subscription).receive
May 02 06:42:06 www1 gopherci[10430]: /data/gopherci-2017-04-27-141809/src/cloud.google.com/go/pubsub/subscription.go:287 +0x26f
bradleyfalzon commented 7 years ago

OK, it appears a pull request comment can have a null position: https://api.github.com/repos/fortytw2/hydrocarbon/pulls/51/comments

[
  {
    "url": "https://api.github.com/repos/fortytw2/hydrocarbon/pulls/comments/114151771",
    "pull_request_review_id": 35595050,
    "id": 114151771,
    "diff_hunk": "@@ -14,10 +15,19 @@ type MockMailer struct {\n func (mm *MockMailer) Send(email string, body string) error {\n \treturn nil\n }\n+func (mm *MockMailer) RootDomain() string {",
    "path": "mailer.go",
    "position": null,
    "original_position": 12,

I'll fix this up, but this also highlights the fact I need better panic handling, which I'll have a closer look at where the best point is for this. As I'd like to mark the build as failed due to internal error, but perhaps continue the panic and let it restart itself back to a known good state.

bradleyfalzon commented 7 years ago

Fixed via 8dff2ea8318145488a3f26485d09f233d31eac43 and better handling of panics in general via 7318f06305f227b9c09ab87bb3523e77e534d8a7. Deploying now.

bradleyfalzon commented 7 years ago

Thanks @fortytw2 shouldn't happen again and we have better handling for these cases now.

bradleyfalzon commented 7 years ago

oh, and kicked off the job again so https://github.com/fortytw2/hydrocarbon/pull/51 is passing not stuck pending.