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

Why does fetchAndStore not close the response body? #29

Open
2785 opened this issue Jun 6, 2024 · 3 comments
Open

Why does fetchAndStore not close the response body? #29

2785 opened this issue Jun 6, 2024 · 3 comments

Comments

@2785
Copy link

2785 commented Jun 6, 2024

Hey,

I see that the fetchAndStore explicitly has a nolint directive on bodyclose - usage of this in jwx's jwk auto refresh is causing a body close memory leak under some... fairly particular circumstances.

Is the expectation that the transformer should close these bodies?

Cheers

@lestrrat
Copy link
Collaborator

lestrrat commented Jun 7, 2024

The exact reasoning escapes me by now, but I believe it was to somehow allow stackable processing. Something like, peek the body, and depending on the body content, decide on a particular processing pattern from a number of choices.

It's possible that it's a moot point by now.

@2785
Copy link
Author

2785 commented Jun 7, 2024

🤔 I think peek -> process style processing in transformer won't interfere with a simple deferred close in that function.

Well unless the transformer manages to capture the raw reader for an outside scope to access? Seems fairly unintended.

Would you take a PR to add a defer close there (and subsequently pulling this into jwx)?

@lestrrat
Copy link
Collaborator

lestrrat commented Jun 8, 2024

yes, but only if it's opt-in. :)

otherwise it will cause problems to those who rely on the current behavior

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

No branches or pull requests

2 participants