search

m-lab / ndt-e2e-clientworker

Code for the client worker of the NDT end-to-end test framework
Apache License 2.0
1 stars 4 forks source link

Fixing race condition in HTTPReplayServer #59

Closed mtlynch closed 8 years ago

mtlynch commented 8 years ago

The ReplayHTTPServer had an issue in that when it started up, it wouldn't necessarily be serving HTTP requests yet because it started the actual servers in background threads. This change explicitly waits until the background threads are actually serving HTTP requests before returning.

Note: This is an uglier and more convoluted solution than I want, but it's the best thing I can think of. I'm open to other solutions that eliminate this race condition.

This change is Reviewable

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.08%) to 88.246% when pulling 4ca066dc85db2a637b2f11bd60e4bd43922d0a23 on mtlynch:fix-http-race-condition into 3f03ccae949d5f83670a4764626ced2d1926479a on m-lab:master.

mtlynch commented 8 years ago

This fixes issue #43

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.04%) to 88.287% when pulling ae858f756f812bdd09023025cf515170727d602e on mtlynch:fix-http-race-condition into 3f03ccae949d5f83670a4764626ced2d1926479a on m-lab:master.

coveralls commented 8 years ago

Coverage Status

Coverage decreased (-0.06%) to 88.266% when pulling 7063b4ccf643dcea28a3da9d4f1137e09013da08 on mtlynch:fix-http-race-condition into 3f03ccae949d5f83670a4764626ced2d1926479a on m-lab:master.

fernandalavalle commented 8 years ago

:lgtm: but just for clarity, what's the specific messiness that bothers you? How we're checking if the server is serving?

Previously, coveralls wrote… > [![Coverage Status](https://coveralls.io/builds/6109320/badge)](https://coveralls.io/builds/6109320) > > Coverage decreased (-0.06%) to 88.266% when pulling **7063b4ccf643dcea28a3da9d4f1137e09013da08 on mtlynch:fix-http-race-condition** into **3f03ccae949d5f83670a4764626ced2d1926479a on m-lab:master**. >

Review status: 0 of 2 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

mtlynch commented 8 years ago

but just for clarity, what's the specific messiness that bothers you? How we're checking if the server is serving?

Yeah, it just feels very ugly that we have to make real HTTP requests to a different port just to make sure the server we started is active. I wish that there was like a thread-safe "is_serving" attribute on the mlab-ns server. For mitmdump I'm a little less embarrassed because we're starting it in a subprocess so there's not much programmatic access we can expect, but I'd still be open to a better solution there.

Review status: 0 of 2 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable