Skip to content
This repository was archived by the owner on Jun 2, 2021. It is now read-only.

use thread.start() to start an actual thread #21

Merged

Conversation

tjstansell
Copy link

thread.run() just runs the code, doesn't spawn off a new thread first.

@wolph
Copy link

wolph commented Nov 15, 2017

lol, that error is hilarious actually :P

Frankly I would be a bit scared of merging this code though... if the code has run like this for a long time it means that it was probably never tested for thread safety and race conditions

@Rockstar04
Copy link
Member

Thanks for the PR! I would have to agree with @wolph though, I think this should be something to definitely cover in #2.

@tjstansell
Copy link
Author

Well, in theory the only things that might be interacting between threads is the logging and the queue where the results are written to. The logging module is designed to be thread-safe and so is the queue.

Do you guys have any suggestions on how to test for thread safety beyond just "trying it"? FWIW, I'll be running with this enabled myself once I finish my changes to support dynamic configs and get this rolled out to monitor our RDS instances.

@wolph
Copy link

wolph commented Nov 18, 2017

I suppose just a bit of trying combined with a quick code check. I agree that both queue and logging should be safe (with logging that's actually debatable but we can ignore that).

Beyond that... I don't really see any obvious problems either but I'll check the code a bit more thoroughly just to be sure

@wolph
Copy link

wolph commented Dec 5, 2017

I've checked the code as far as I can and I don't see any obvious problems. It might cause issue in some logging configurations but in general it will probably work just fine :)

@wolph wolph requested a review from Rockstar04 December 5, 2017 14:39
@Rockstar04 Rockstar04 merged commit 781259d into NewRelic-Python-Plugins:master Dec 5, 2017
@Rockstar04
Copy link
Member

We can tag the next release as a major version bump if we are worried about the backwards compatibility this introduces.

@tjstansell
Copy link
Author

FWIW, I haven't seen any issues with this in all of my testing I've done so far... thanks for merging!

@tjstansell tjstansell deleted the use_threading branch January 22, 2018 18:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants