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

Stream automatically gets closed. Can we make this optional? #650

Closed
peter-shinydocs opened this issue Sep 23, 2021 · 13 comments
Closed

Stream automatically gets closed. Can we make this optional? #650

peter-shinydocs opened this issue Sep 23, 2021 · 13 comments

Comments

@peter-shinydocs
Copy link

This is generally an amazing idea. But it would be nice if we could choose to save the stream.

request.Dispose();

@tmenier
Copy link
Owner

tmenier commented Sep 29, 2021

You want to save the request stream? Can you elaborate on what you're trying to do?

@peter-shinydocs
Copy link
Author

peter-shinydocs commented Sep 29, 2021 via email

@tmenier
Copy link
Owner

tmenier commented Sep 30, 2021

I'm not sure how you'd do that without reading the whole file into memory. Otherwise, even if Flurl didn't dispose the request, I don't think you'd be able to read the stream twice. If it's not a huge file, I'd read it into a byte array and send that twice. If it's too big for that, just read it off disk (or wherever it's coming from) twice. If you think I'm missing something, maybe a code snippet would help.

@peter-shinydocs
Copy link
Author

peter-shinydocs commented Sep 30, 2021 via email

@tmenier
Copy link
Owner

tmenier commented Sep 30, 2021

If it's over 2gb I'd really encourage you to keep it simple and just read it from storage twice.

@tmenier tmenier closed this as completed May 6, 2022
@ArturMarekNowak
Copy link

ArturMarekNowak commented Apr 5, 2023

@tmenier but if it is a temporary Filestream which I want to delete on close, lack of the ability to keep it open after request is problematic. Of cource I could make deep copy just before the request but it would double RAM used. It would be much simpler if flurl simply didn't dispose it

@tmenier
Copy link
Owner

tmenier commented May 23, 2023

Re-opening, I'll give this a fresh look for 4.0. Need to re-evaluate whether this is the right place to dispose the request (if at all), whether streams should be handled differently, or to your point whether it should somehow be optional.

@tmenier tmenier reopened this May 23, 2023
@tmenier tmenier added this to Backlog in Default Project via automation May 23, 2023
@tmenier tmenier moved this from Backlog to 4.0 in Default Project Sep 11, 2023
@tmenier
Copy link
Owner

tmenier commented Sep 18, 2023

Sorry for the long delay on this one. I'm of the opinion that the Dispose call can simply be dropped.

aspnet/Security#886

It should be assumed by the developer that if they provide a stream for the request content, they are on the hook for managing it. I'll drop that line for 4.0.

@tmenier tmenier moved this from 4.0 to Ready in Default Project Sep 18, 2023
@tmenier tmenier moved this from Ready to In Prerelease in Default Project Oct 9, 2023
@sanyappc
Copy link

Maybe we should add Dispose here now?

@tmenier
Copy link
Owner

tmenier commented Oct 16, 2023

@sanyappc What would be the reason for doing that? Can you explain?

@sanyappc
Copy link

@tmenier The PostMultipartAsync method creates CapturedMultipartContent itself, so I think it should Dispose it after executing SendAsync

I thought so at first, but now I'm not so sure

@tmenier
Copy link
Owner

tmenier commented Oct 17, 2023

@sanyappc I think the same argument for not disposing applies if you're doing multipart - it's only important if you're providing a stream, and you may or may not want it closed immediately.

@tmenier tmenier moved this from In Prerelease to Released in Default Project Dec 14, 2023
@tmenier tmenier closed this as completed Dec 14, 2023
@petervanleeuwen
Copy link

Thank you! Sorry I wish I could have made an attempt.

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

No branches or pull requests

5 participants