Skip to content

Conversation

pilif
Copy link
Contributor

@pilif pilif commented Sep 22, 2015

As I've said on the mailing list, I'm not 100% sure whether this is the correct usage of the fastcgi API, so I would really appreciate if somebody with fastcgi (and FPM) knowledge could give a review.

Furthermore, if somebody could give me some pointers in how to write tests for FPM, I will gladly provide tests for this too.

This fixes bug 70279:

in commit f20118a the call to fcgi_hash_init() moved out of the actual request handling to the point where FPM was only calling it once when it spawns a subprocess.

Because the datastructure was not cleaned between handling multiple requests, headers and other data of previous requests were not cleaned correctly.

This patch now calls fcgi_hash_clean() before handling individual requests at the cost of a bit of the performance gains of the previous commit.

in commit f20118a the call to
fcgi_hash_init() moved out of the actual request handling to the point
where FPM was only calling it once when it spawns a subprocess.

Because the datastructure was not cleaned between handling multiple
requests, headers and other data of previous requests were not cleaned
correctly.

This patch now calls fcgi_hash_clean() before handling individual
requests at the cost of a bit of the performance gains of the previous
commit.
@remicollet
Copy link
Member

Would it be possible to add a test for this ?

@pilif
Copy link
Contributor Author

pilif commented Sep 22, 2015

I would love to, if you could point me at some existing tests for FPM - it's currently unclear to me how FPM is being tested.

@laruence
Copy link
Member

hmm, I already fixed it in 1d6228b thanks

@pilif
Copy link
Contributor Author

pilif commented Sep 22, 2015

closing this one then.

@pilif pilif closed this Sep 22, 2015
@pilif
Copy link
Contributor Author

pilif commented Sep 22, 2015

Question: Wouldn't my solution here be a slight bit better? After 1d6228b the request gets destroyed completely, but for some reason FPM wasn't doing that before - possibly for performance reasons. Just cleaning the request structure before handling the request seems less intrusive to me.

Bear in mind though that I practically have no clue - just thinking aloud here.

@pilif
Copy link
Contributor Author

pilif commented Sep 22, 2015

ah. right. Yeah - I see that destroy basically only calls fcgi_hash_clean() - so yeah - your patch is better.

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.

3 participants