Config map length fix, fix bug with missing events by closing the websocket #367
Conversation
…s possible if config is overriden via admin API and merged in with viper nested setter syntax Signed-off-by: jebonfig <joe.bonfiglio@kaleido.io>
…connection needs to be closed when event stream loop terminates to prevent the dangling connection consuming events Signed-off-by: jebonfig <joe.bonfiglio@kaleido.io>
jebonfig
requested review from
awrichar,
nguyer and
peterbroadhurst
as code owners
Codecov Report
@@ Coverage Diff @@
## main #367 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 239 239
Lines 12888 12890 +2
=========================================
+ Hits 12888 12890 +2
Continue to review full report at Codecov.
|
|
|
||
| func (e *Ethereum) eventLoop() { | ||
| defer close(e.closed) | ||
| defer e.wsconn.Close() |
There was a problem hiding this comment.
This looks correct to me. I'm now wondering what is the purpose of close(e.closed) on the line above this? Perhaps that's a @peterbroadhurst question?
Also, I assume we need this same fix in the Fabric plugin?
There was a problem hiding this comment.
The e.closed is to account for Unit Tests waiting for the event loop to end before exiting, which is important otherwise you can get spill-over of execution after the end of tests, calling mocks after they are cancelled.
It does feel like the e.wsconn.Close() should come before the close(e.closed) to me (think the risk of spurious UT failures is low, but I couldn't rule them out)
There was a problem hiding this comment.
Also, I assume we need this same fix in the Fabric plugin?
Agreed. Think we should get the close into both - sorry it's missing, surprised at that and how it hasn't caused problems before 🤷
Signed-off-by: jebonfig <joe.bonfiglio@kaleido.io>
4 participants
No description provided.