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

Wip : Make request thread safe #293

Closed
wants to merge 2 commits into from

Conversation

Rohith-Raju
Copy link
Contributor

Initial commit to making request thread-safe

In this implementation, I Introduced a new request object whenever expect() is called to avoid data race.
why expect?
Since expect() is the one that transfers the request object to matches and transformers.. we can supply a new Request object to them so that each go-routine has its own copy.

Things that are going wrong here are

  • While creating a new request object, we are also copying locks.
  • checkOrder relies on struct var expectCalled and since a new Request object is being created, expectCalled is not being updated and hence fails.
  • Request_Redirects also fails

@Rohith-Raju Rohith-Raju marked this pull request as draft February 15, 2023 14:49
@Rohith-Raju
Copy link
Contributor Author

Ping: @gavv

@gavv
Copy link
Owner

gavv commented Feb 23, 2023

Hi,

So... this patch is broken in many ways:

  1. The patch is doing a shallow copy of Request, but there is still mutable state shared across these shallow copies: httpReq, transformers and matchers slices, multipart writer, etc, etc. So this copying didn't actually make anything thread-safe.

  2. On the other hand, Request object is not supposed to be copyable, as you can see from noCopy field. That's why some tests broke. I wonder why you didn't try to address the failures though.

  3. There is a requirement that Expect can be called only once. Actually, trying to parallelize Expect calls is a dead-end approach in general, parallel executions of Expect should not be allowed at all.

  4. As you correctly mentioned, copying locks is forbidden and is a bug. Again, I wonder why you didn't try to fix it before submitting PR.

BTW I'd recommend to do self-review of your code before submitting it. There are some clear copy-paste errors, like var resp Request or using RWMutex instead of Mutex. This is usually a bad sign for reviewer.

I'll do the rest of the task by myself. Thanks for PR, anyway.

@gavv gavv closed this Feb 23, 2023
@gavv gavv mentioned this pull request Feb 23, 2023
8 tasks
@gavv gavv linked an issue Feb 23, 2023 that may be closed by this pull request
8 tasks
@gavv gavv removed a link to an issue Feb 23, 2023
8 tasks
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.

2 participants