lorenzodonini / ocpp-go

Open Charge Point Protocol implementation in Go
MIT License
275 stars 126 forks source link

Need to support panic recovery in the request handlers #205

Open ysaakpr opened 1 year ago

ysaakpr commented 1 year ago

https://github.com/lorenzodonini/ocpp-go/blob/c34f6ad20983d34d7575d9fa05d85368ed0109e2/ocpp1.6/central_system.go#L502-L509

What happen if a panic happens in a request handler in this go routine?

My server keep re starting since its a go routine, and my request level panic handler not working here. Need a way to handle the panic situations. Or have proper panic recovery for all the internal go routines.

defer func() {
            r := recover()
            if r != nil {
                var err error
                switch t := r.(type) {
                case string:
                    err = errors.New(t)
                case error:
                    err = t
                default:
                    err = errors.New("Unknown error")
                }
                // sendMeMail(err)
                fmt.Println("recivered panic:", err)
                cs.sendResponse(chargePoint.ID(), confirmation, err, requestId)
            }
        }()

This is what is did in the start of the go routine to fix it.

lorenzodonini commented 1 year ago

The callback expects either a response or an error. Currently a panic within the same goroutine will break the request - response flow for that connection (which can only be fixed with a re-connection).

Why does the handler panic anyway?

ysaakpr commented 1 year ago

the panic will cause the system to exit if not recovered.

Why panic, may be a bad coder/coding. and some libraries cause panic instead of error in its error cases, and unless handled carefully all these can cause the entier server to go down. Which is not a good behaviour

Isn't it better to recover the panic, or give the flexibility to handle a panic at root level, by setting a panic handler.

lorenzodonini commented 1 year ago

My point was simply about "bad coders" being responsible for their own implementation. If a handler is panicking, it should be fixed in there.

That being said your suggestion is definitely a good improvement, I'll add it in a PR.

ysaakpr commented 1 year ago

My point was simply about "bad coders" being responsible for their own implementation. If a handler is panicking, it should be fixed in there.

Yes... it should be fixed in there, in our case there are many handlers performing ocpp message handling, and there is no common place in the library where i can wrap the logic of panic recovery. From a library perspective we need some handler injection possible at the root level,

That being said your suggestion is definitely a good improvement, I'll add it in a PR.

Thanks for taking it, better to accept it as a panic handler in the configurations of Server