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 mutex to FIFOResponseHandler #29

Merged

Conversation

mheck136
Copy link
Contributor

@mheck136 mheck136 commented Apr 6, 2022

Fix possible data race in FIFOResponseHandler.ServeHTTP by adding a
mutex.

Addresses #28

Fix possible data race in FIFOResponseHandler.ServeHTTP by adding a
mutex.
@migueleliasweb
Copy link
Owner

Hi @mheck136 , thanks for the PR!

Could you also add the mutex to the PaginatedReponseHandler?

@mheck136
Copy link
Contributor Author

mheck136 commented Apr 6, 2022

Hi @migueleliasweb, I don't think that there is any risk of a data race for PaginatedResponseHandler. I can still add the mutex if you want me to.

@migueleliasweb
Copy link
Owner

Hi @migueleliasweb, I don't think that there is any risk of a data race for PaginatedResponseHandler. I can still add the mutex if you want me to.

It's all good, let me double check a couple things and I will be right back with you.

@migueleliasweb
Copy link
Owner

Hi @mheck136 , yeah we probably won't need the mutex in the paginated handler for now.

Can you just merge the two defer statements into one? (using the last one where the index is incremented).

Then I can merge and create a new release :)

@mheck136
Copy link
Contributor Author

@migueleliasweb, I'm not sure that's a good idea. I would keep the defer srh.lock.Unlock() right after the Lock(), because they are directly related to each other. This way it's also easier to maintain the right ordering of the deferred actions.
If you still want me to change it, I can do that. Please let me know.

@mheck136
Copy link
Contributor Author

mheck136 commented Jun 8, 2022

@migueleliasweb , how do you want to proceed with this PR?

@mheck136
Copy link
Contributor Author

@migueleliasweb , I still think it would be best to keep the code as it is. When defer srh.lock.Unlock() is merged with defer func()... later on and the panic in line 40 triggers, the lock won't be released again, which should be avoided. Would be great if we could get this merged :)

@migueleliasweb
Copy link
Owner

Hi @mheck136 , sorry for the delay. Life has been pretty busy lately.

Let's merge this 👍

@migueleliasweb migueleliasweb merged commit 122621d into migueleliasweb:master Jul 18, 2022
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

2 participants