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

Fix race conditions #536

Closed
na-- opened this issue Mar 16, 2018 · 2 comments
Closed

Fix race conditions #536

na-- opened this issue Mar 16, 2018 · 2 comments
Assignees
Milestone

Comments

@na--
Copy link
Member

na-- commented Mar 16, 2018

Currently when the test suite is run with the -race flag, a few race conditions appear. I think that one of them is this (or related to it), while another one seems to be related to metrics collection of http requests that reuse the same connection.

The end goal here is to enable the -race flag for CI tests runs so we don't regress in the future.

@na-- na-- added the bug label Mar 16, 2018
@na-- na-- added this to the v0.21.0 milestone Mar 16, 2018
@na-- na-- mentioned this issue Mar 16, 2018
4 tasks
@na-- na-- self-assigned this Mar 26, 2018
@na--
Copy link
Member Author

na-- commented Mar 26, 2018

The metrics collection race condition is because we use httptrace.ClientTrace incorrectly. From the docs:

Functions may be called concurrently from different goroutines and some may be called after the request has completed or failed.

Regarding ClientTrace.ConnectStart and ClientTrace.ConnectDone specifically:

If net.Dialer.DualStack (IPv6 "Happy Eyeballs") support is this may be called multiple times.

And ClientTrace.WroteRequest:

It may be called multiple times in the case of retried requests.

The k6 code that uses ClientTrace has a lot of reads and writes for the same variables without any synchronization, so it has to be refactored. I'm not sure that a bunch of mutexes is the correct approach here, something event-based seems like a better solution, for example each of the ClientTrace methods could just create an event, send it over a channel and something else can synchronously and safely process them.

@na--
Copy link
Member Author

na-- commented Apr 4, 2018

Closed by #564, if any new race conditions are uncovered by #537, they will either be directly fixed if they are simple or get their own issues.

@na-- na-- closed this as completed Apr 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants