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.

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

Conversation

dammarco
Copy link

When testing the flakyness of our OCPP unit tests we found a few races happening when reconnection of OCPP clients happens really fast.

Bugs

Websocket: Panic Send on closed channel

Websockets error channel should only be used when the channel is not closed already (1f0c95e)

When shutting down Central System, errors are sent on a closed channel

When shutting down Central System, the error channel might be closed right before we try to send something on it. This is checked before now and protected with a lock. (09198de)

Dispatcher Disconnect Handling

When a client disconnects and connectrs really fast, the dispatcher is locked in a state where he is waiting for a request to be completed. The request got removed from the queue though and thus, the dispatcher is not able to find a matching request when the real client answers to the message. A cleaner way to handle the disconnection of clients was proposed. (5bb5806)

Dispatcher nil reference when peeking element from queue

Fix nil reference when the queue peeks a nil element (ddc83d7)

Marco Trettner added 4 commits January 24, 2023 08:24
…tem is shutting down and still tries to send an error on a channel that has been closed.
…cts too fast ends up blocking the dispatchers messagePump until a timeout (30s) happens.
@dammarco dammarco added the bug Something isn't working label Jan 30, 2023
@dammarco dammarco requested review from gq0 and guelfey January 30, 2023 08:30
@dammarco dammarco self-assigned this Jan 30, 2023
@dammarco dammarco changed the title Feat/fix several race conditions Fix Several Race Conditions Jan 30, 2023
@coveralls
Copy link

Pull Request Test Coverage Report for Build 4041955883

  • 43 of 65 (66.15%) changed or added relevant lines in 3 files are covered.
  • 12 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.3%) to 87.383%

Changes Missing Coverage Covered Lines Changed/Added Lines %
ws/websocket.go 3 5 60.0%
ocppj/dispatcher.go 36 42 85.71%
ocpp1.6/central_system.go 4 18 22.22%
Files with Coverage Reduction New Missed Lines %
ocpp2.0.1/v2.go 2 66.67%
ws/websocket.go 2 90.0%
ocppj/dispatcher.go 8 89.46%
Totals Coverage Status
Change from base Build 3627954280: -0.3%
Covered Lines: 6323
Relevant Lines: 7236

💛 - Coveralls

case <-cs.errC:
// channel is closed, don't send the error
default:
cs.errC <- err
Copy link
Member

@guelfey guelfey Jan 30, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants