Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Several Race Conditions #16

Open
wants to merge 4 commits into
base: gridx_extensions
Choose a base branch
from
Open

Fix Several Race Conditions #16

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1f0c95e
fix(ws): only send the websocket error when the channel is not closed…
Jan 24, 2023
09198de
fix(ocpp1.6): prevent panic "send on closed channel" when central sys…
Jan 25, 2023
5bb5806
fix(ocppj): fix race condition when a disconnected client who reconne…
Jan 25, 2023
ddc83d7
fix(ocppj): prevent nil reference panic when the map peeks a nil element
Jan 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix(ocpp1.6): prevent panic "send on closed channel" when central sys… 
…tem is shutting down and still tries to send an error on a channel that has been closed.
  • Loading branch information
Marco Trettner committed Jan 25, 2023
commit 09198debd0c5d8ec56029229b61c0a32beebc2e1
23 changes: 22 additions & 1 deletion ocpp1.6/central_system.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package ocpp16

import (
"fmt"
"sync"

"github.com/lorenzodonini/ocpp-go/internal/callbackqueue"
"github.com/lorenzodonini/ocpp-go/ocpp"
Expand All @@ -26,6 +27,7 @@ type centralSystem struct {
smartChargingHandler smartcharging.CentralSystemHandler
callbackQueue callbackqueue.CallbackQueue
errC chan error
errCLock sync.Mutex
}

func newCentralSystem(server *ocppj.Server) centralSystem {
Expand All @@ -40,7 +42,18 @@ func newCentralSystem(server *ocppj.Server) centralSystem {

func (cs *centralSystem) error(err error) {
if cs.errC != nil {
cs.errC <- err
// It can happen that the error channel is getting closed
// when the central system is shutting down.
// If this happens right before this call here, we have a closed channel.
// To prevent a panic, we check if it is closed before sending the error.
cs.errCLock.Lock()
defer cs.errCLock.Unlock()
select {
case <-cs.errC:
// channel is closed, don't send the error
default:
cs.errC <- err
Copy link
Member

@guelfey guelfey Jan 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally it's problematic if you have a Goroutine closing a channel that another Goroutine can write to. Not too familiar with the rest of the code anymore, but is there any reason we need to close errC at all? It's mostly used for logging, right? Then I don't see a reason why we can't just keep it open. Writers could just do a select and throw the error away if the reader Goroutine already stopped.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea 🤔 I'm also not familiar with it, I just fixed the issues :D
how would the writer know if the readers goroutine has stopped?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The write to the channel would block, but you can just wrap it in a select with a default case to prevent it from causing an issue

}
}
}

Expand Down Expand Up @@ -400,6 +413,14 @@ func (cs *centralSystem) SendRequestAsync(clientId string, request ocpp.Request,

func (cs *centralSystem) Start(listenPort int, listenPath string) {
cs.server.Start(listenPort, listenPath)

// Make sure to lock the access to the error channel to prevent races/panics
// When an error happens right after the server is shutting down.
if cs.errC != nil {
cs.errCLock.Lock()
defer cs.errCLock.Unlock()
close(cs.errC)
}
}

func (cs *centralSystem) sendResponse(chargePointId string, confirmation ocpp.Response, err error, requestId string) {
Expand Down