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

Add preliminary check before registering new receivers in the log handler #173

Merged
merged 4 commits into from
Feb 4, 2022

Conversation

Aayyush
Copy link

@Aayyush Aayyush commented Feb 3, 2022

We currently don't have a check in place before we register new web socket connections. As a result, if someone were to visit the jobs URL with invalid job ID, we would still register the connection and create go channels which could cause memory issues in the future.

So, we add a check to ensure a valid job exists in our log handler before we register new receivers.

Comment on lines 121 to 123
defer func() {
p.receiverBuffersLock.RUnlock()
}()
Copy link

Choose a reason for hiding this comment

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

don't think you need to wrap it into anonymous function here.

Copy link
Author

Choose a reason for hiding this comment

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

Yup! I've removed it :)

msarvar
msarvar previously approved these changes Feb 3, 2022
Copy link

@msarvar msarvar left a comment

Choose a reason for hiding this comment

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

looks good overall

@Aayyush Aayyush merged commit 316147d into release-v0.17.3-lyft.1 Feb 4, 2022
@Aayyush Aayyush deleted the aay/register-receiver-fix branch February 4, 2022 19:55
Aayyush added a commit that referenced this pull request Feb 14, 2022
* Add UUID for Log Streaming Job ID (#167)

* Update log handler to close buffered  channels when an operation is complete (#170)

* Add preliminary check before registering new receivers in the log handler (#173)

* Using projectOutputBuffers to check for jobID instead of receiverBuffers (#181)

* Refactor log handler  (#175)

* Reverting go.mod and go.sum

* Fix lint errors

* Fix linting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants