Skip to content

fix #1501 ssh hangs caused by #1461 #1513

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 19, 2017
Merged

fix #1501 ssh hangs caused by #1461 #1513

merged 1 commit into from
Apr 19, 2017

Conversation

lunny
Copy link
Member

@lunny lunny commented Apr 19, 2017

No description provided.

@lunny lunny added the type/bug label Apr 19, 2017
@lunny lunny added this to the 1.2.0 milestone Apr 19, 2017
@lunny lunny added the issue/critical This issue should be fixed ASAP. If it is a PR, the PR should be merged ASAP label Apr 19, 2017
@bkcsoft
Copy link
Member

bkcsoft commented Apr 19, 2017

LGTM

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 19, 2017
@lunny
Copy link
Member Author

lunny commented Apr 19, 2017

About the hang reason: when pull or push via SSH, models.GetUserByName will be invoked and then https://github.com/go-gitea/gitea/blob/master/models/user.go#L166 will be invoked. But on NewSanitizer function log.Trace will output something to the os.Stdout. This is the reason which hang the SSH. So don't output any log to os.Stdout when SSH process. Any idea to avoid this situation next?

@tboerger tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 19, 2017
@lunny
Copy link
Member Author

lunny commented Apr 19, 2017

The direction I have said that is moving all database operations to internal HTTP operations. I will send another PRs to do that.

@strk
Copy link
Member

strk commented Apr 19, 2017

Sorry but I don't get why outputting to os.Stdout hangs, is there a deadlock ?

@appleboy
Copy link
Member

LGTM

@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 19, 2017
@lunny
Copy link
Member Author

lunny commented Apr 19, 2017

@strk I think maybe the extra output to os.Stdout lets other command wait until the output is end. But this is not the expected output so it's hangs.

@lunny lunny merged commit a2d365c into go-gitea:master Apr 19, 2017
@lunny lunny deleted the lunny/fix_ssh_hang branch April 19, 2017 11:20
@tbaschak
Copy link

tbaschak commented Apr 19, 2017 •
edited
Loading

This pull fixes my SSH issues!

@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@appleboy appleboy appleboy approved these changes

Assignees
No one assigned
Labels
issue/critical This issue should be fixed ASAP. If it is a PR, the PR should be merged ASAP lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Milestone
1.2.0
Development

Successfully merging this pull request may close these issues.
6 participants
@lunny @bkcsoft @strk @appleboy @tbaschak @tboerger