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 High CPU occupancy #13

Merged
merged 2 commits into from
Mar 19, 2018
Merged

Fix High CPU occupancy #13

merged 2 commits into from
Mar 19, 2018

Conversation

kimw
Copy link
Contributor

@kimw kimw commented Mar 19, 2018

The dead loop causes high CPU occupancy. Quit loop when no more byte to copy.

The dead loop causes high CPU occupancy. Quit loop when no more byte to copy.
@kimw
Copy link
Contributor Author

kimw commented Mar 19, 2018

fix #12

@kimw kimw changed the title Fix #12 Fix High CPU occupancy Mar 19, 2018
@kimw
Copy link
Contributor Author

kimw commented Mar 19, 2018

@cbeuw
I don't know go lang in fact. Just learned in about 30mins to debug #12. If I didn't fix the bug in golang way, I'm sorry.

BTW, should the (pair *webPair) serverToRemote() do the same change?

As from,

func (pair *webPair) serverToRemote() {
	_, err := io.Copy(pair.remote, pair.webServer)
	if err != nil {
		pair.closePipe()
	}
}

To,

func (pair *webPair) serverToRemote() {
	for {
		length, err := io.Copy(pair.remote, pair.webServer)
		if err != nil || length == 0 {
			pair.closePipe()
		}
	}
}

@cbeuw cbeuw merged commit 5821ef0 into cbeuw:master Mar 19, 2018
@cbeuw
Copy link
Owner

cbeuw commented Mar 19, 2018

Yes serverToRemote needs to do the same.

I thought io.Copy would block when there's nothing to read. I tested the bypass feature before and I had probably completely overlooked the CPU issue. I hope people who are using gq-server on their VPS didn't turn on automatic CPU scaling, or that would be a lot of money wasted.

I didn't know either how I missed the for loop in serverToRemote.

Thank you very much for your contribution

@kimw kimw deleted the fix-12 branch March 29, 2018 03:47
@kimw kimw mentioned this pull request Mar 29, 2018
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.

None yet

2 participants