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

Concurrent Websocket Handlers for Log Streaming #7428

Merged
merged 18 commits into from
Oct 6, 2020

Conversation

rauljordan
Copy link
Contributor

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

Our current log streaming server has serious issues when it comes to dealing with concurrent connections, as it handles everything in a single thread, blocking the program if there is a secondary connection or if a request times out. This PR adds concurrency via a simple map + mutex combo to keep track of connected ws clients and send log messages to them accordingly. Additionally, subscribing to an event feed of log messages should not happen in the connection handler but rather in some goroutine which runs in the server's main loop.

Tested with 5 concurrent connections with manual disconnects and processes kept going as expected

@rauljordan rauljordan requested a review from a team as a code owner October 3, 2020 21:14
shared/logutil/stream.go Outdated Show resolved Hide resolved
shared/logutil/stream.go Outdated Show resolved Hide resolved
rauljordan and others added 3 commits October 3, 2020 16:45
Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>
Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>
prestonvanloon
prestonvanloon previously approved these changes Oct 3, 2020
@codecov
Copy link

codecov bot commented Oct 6, 2020

Codecov Report

Merging #7428 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #7428   +/-   ##
=======================================
  Coverage   60.27%   60.27%           
=======================================
  Files         424      424           
  Lines       30565    30565           
=======================================
  Hits        18423    18423           
  Misses       9123     9123           
  Partials     3019     3019           

@prylabs-bulldozer prylabs-bulldozer bot merged commit 4d77978 into master Oct 6, 2020
@delete-merged-branch delete-merged-branch bot deleted the concurrent-wss branch October 6, 2020 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Api related tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants