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 test cases for StreamRequestBody #915

Merged
merged 6 commits into from Dec 9, 2020
Merged

Add test cases for StreamRequestBody #915

merged 6 commits into from Dec 9, 2020

Conversation

kiyonlin
Copy link
Contributor

@kiyonlin kiyonlin commented Nov 19, 2020

Co-authored-by: Fenny fenny@gofiber.io

Related to #911

server_test.go Show resolved Hide resolved
@kiyonlin kiyonlin marked this pull request as draft November 20, 2020 01:09
@Fenny
Copy link
Contributor

Fenny commented Nov 30, 2020

@erikdubbelboer, do you mind reviewing the changes?

server_test.go Outdated
if err != nil {
t.Error(err)
}
case <-time.After(100 * time.Millisecond):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This timeout should be higher as it is constantly failing on travis-ci.

server_test.go Outdated Show resolved Hide resolved
server_test.go Show resolved Hide resolved
@kiyonlin
Copy link
Contributor Author

kiyonlin commented Dec 5, 2020

When I don’t use done chan, the test occasionally fails. I suspect it is related to reading and writing rw.r at the same time.

server_test.go Outdated
Handler: func(ctx *RequestCtx) {
checkReader(t, ctx.RequestBodyStream(), part1)
close(next)
<-done
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of adding the done channel I think it's probably a better idea to add a sync.Mutex to readWriter to make its operations safe just like reading and writing on a normal socket would be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can't use sync.Mutex. If handler get lock first, then second part can't be sent, and the test case will timeout.
So we should use a more real Conn. Let me try it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use fasthttputil.NewPipeConns() now.

server_test.go Outdated
if _, err := cc.Write([]byte(fmt.Sprintf("POST /foo2 HTTP/1.1\r\nHost: aaa.com\r\nContent-Length: %d\r\nContent-Type: aa\r\n\r\n%s", contentLength, part1))); err != nil {
t.Error(err)
}
}()
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no reason to do this in a goroutine. If you move the ch <- s.ServeConn(sc) gorourine up to before this Write then you don't have to do this Write or the one below it in goroutines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, thanks!

@kiyonlin kiyonlin marked this pull request as ready for review December 9, 2020 00:09
@kirillDanshin kirillDanshin merged commit 0e23da7 into valyala:issue-622 Dec 9, 2020
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

4 participants