Skip to content

fix dup log race condition on shutdown#40

Merged
JeffreyLMelvin merged 2 commits intozach-taylor:masterfrom
JeffreyLMelvin:fix_dup_log_race_condition
Oct 9, 2020
Merged

fix dup log race condition on shutdown#40
JeffreyLMelvin merged 2 commits intozach-taylor:masterfrom
JeffreyLMelvin:fix_dup_log_race_condition

Conversation

@JeffreyLMelvin
Copy link
Copy Markdown
Collaborator

@JeffreyLMelvin JeffreyLMelvin commented Oct 2, 2020

Problem

wait_until_empty allows the code to wait until the queue has been emptied. However, there is still a race condition between the response from the Splunk server and the execution of shutdown.

Instance shutdown() calls _splunk_worker(), which will call empty_queue(). If the connection to splunk is still open from the Timer call that emptied the queue, the Timer thread will not have yet cleared self.log_payload, which can result in the shutdown resending the payload, because empty_queue() only appends to self.log_payload.

Solution

There are multiple options:

  1. Have _splunk_worker() check the queue size. This was not done as it doesnt align with the option to send a payload as an arg to the function
  2. Have shutdown() send the payload in the _splunk_worker() call.
  3. Clear self.log_payload as soon as it is read instead of after it is received by splunk. The payload is thrown away regardless of error already, so this seemed like the best option.

Additionally removed the flush_interval adjustment made in wait_until_empty() as the timer is already adjusted in _splunk_worker() when there is content in the queue after completing a call to splunk

@zach-taylor

Copy link
Copy Markdown
Owner

@zach-taylor zach-taylor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks okay to me 👍

@JeffreyLMelvin
Copy link
Copy Markdown
Collaborator Author

Ok, I will merge and cut a release

@JeffreyLMelvin JeffreyLMelvin merged commit 0cf5bad into zach-taylor:master Oct 9, 2020
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