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

🐛 [Bug]: can't close stream after sending via c.SendStream #3302

Open
3 tasks done
KernelDeimos opened this issue Feb 9, 2025 · 3 comments
Open
3 tasks done

🐛 [Bug]: can't close stream after sending via c.SendStream #3302

KernelDeimos opened this issue Feb 9, 2025 · 3 comments

Comments

@KernelDeimos
Copy link

KernelDeimos commented Feb 9, 2025

Bug Description

I have an implementer of io.ReadCloser. I wish to stream the contents from this reader to the client and then close the reader after this is done. However. c.SendStream doesn't block or provide any way for me to know when the contents are done streaming, so this is actually impossible.

I had just migrated all my code from Gin because something in Gin turned out to be impossible, so this subsequent blocker definitely sucks.
I was able to get it working

defer myReadCloser.Close()
_, err = io.Copy(c.Response().BodyWriter(), myReadCloser)
if err != nil {
  return err
}

This was definitely very counterintuitive so I will leave this issue open (UX issues are still issues, right?). I also want to say thanks to the maintainers - the migration from Gin to Fiber was a little rough, but it has already allowed me to do two things that Gin wasn't capable of.

How to Reproduce

described in bug description

Expected Behavior

described in bug description

Fiber Version

v2.52.6

Code Snippet (optional)

Checklist:

  • I agree to follow Fiber's Code of Conduct.
  • I have checked for existing issues that describe my problem prior to opening this one.
  • I understand that improperly formatted bug reports may be closed without explanation.
Copy link

welcome bot commented Feb 9, 2025

Thanks for opening your first issue here! 🎉 Be sure to follow the issue template! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

@KernelDeimos
Copy link
Author

On Discord someone pointed out that there's a type-assertion to io.ReadCloser and it actually closes the stream for me. I'm not sure if this is idiomatic - I would assume if I opened a resource I'm responsible for closing it; what happens if I want to do other things with that stream too? I might not expect it to get closed by SendStream.

@JIeJaitt
Copy link
Contributor

JIeJaitt commented Feb 13, 2025

Hi there!

This behavior is actually inherited from the underlying fasthttp library that Fiber is built upon. When you use c.SendStream(), fasthttp will automatically call Close() on any stream that implements io.Closer after the content has been fully read.

Here's the relevant code from fasthttp that handles this behavior(fasthttp/http.go):

func (resp *Response) closeBodyStream(wErr error) error {
	if resp.bodyStream == nil {
		return nil
	}
	var err error
	if bsc, ok := resp.bodyStream.(io.Closer); ok {
		err = bsc.Close()
	}
	if bsc, ok := resp.bodyStream.(ReadCloserWithError); ok {
		err = bsc.CloseWithError(wErr)
	}
	if bsr, ok := resp.bodyStream.(*requestStream); ok {
		releaseRequestStream(bsr)
	}
	resp.bodyStream = nil
	return err
}

This is different from other Go web frameworks like Gin or Echo that are built on top of net/http, where stream management is left to the user. The automatic closing behavior is specific to fasthttp's design decisions.

I don't think this qualifies as a bug since it's an intentional design choice in fasthttp. If you feel strongly about this behavior, you might want to raise this discussion in the fasthttp repository instead, as this is core to how fasthttp handles streams. What do the seniors think of this idea? @ReneWerner87 @efectn @gaby

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

No branches or pull requests

2 participants