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

adding multithreading tests for batch processor #947

Closed

Conversation

aravinsiva
Copy link
Contributor

Add's concurrency tests to ensure that force_flush() and shutdown() could be called concurrently without causing racing coniditions.

Addresses:
#392

@aravinsiva aravinsiva requested a review from a team July 27, 2020 19:39
@aravinsiva aravinsiva changed the title adding multithreading tests for batch processor [WIP] adding multithreading tests for batch processor Jul 28, 2020
span_names1 = ["yyy", "baz", "fox"]

for name in span_names0:
_create_start_and_end_span(name, span_processor_to_flush)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not super familiar with this code, but what happens if spans are "created" by one thread while the other thread is shutting down? Is that something we should test as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a great point. The only reason I didn't look into that in particular is because the two methods outlined in the issue were force_flush() and shutdown()

Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

What is the race condition we are testing here?

@lzchen
Copy link
Contributor

lzchen commented Aug 13, 2020

@aravinsiva
Are you still working on this? Can you answer @aabmass 's question?

@aravinsiva
Copy link
Contributor Author

aravinsiva commented Aug 14, 2020

@aravinsiva
Are you still working on this? Can you answer @aabmass 's question?

My mistake have been busy tackling another project. And it is currently testing that these can be called concurrently without causing a change in the end resulting spans. Feel free to let me know if this is not the correct approach to test what we want to be tested.

Comment on lines +173 to +174
span_processor_to_shutdown.shutdown()
self.assertTrue(span_processor_to_flush.force_flush())
Copy link
Member

Choose a reason for hiding this comment

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

@lzchen is this what you meant by:

I believe they are referring to the behaviour for shutdown and force_flush when they are called at the same time in different threads (e.g. multiple BatchSpanProcessors)

It looks like shutdown() would block until its done exporting

def shutdown(self) -> None:
# signal the worker thread to finish and then wait for it
self.done = True
with self.condition:
self.condition.notify_all()
self.worker_thread.join()
self.span_exporter.shutdown()

So they wouldn't be called at the same time unless you actually did it in separate threads. Even if you did it in separate threads, the test might be flaky since you can't guarantee the order of execution.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @Oberon00 's comment was referring to concurrency cases in general (calling force_flush manually while the span processor is exporting, calling shutdown while the processor is exporting, etc.).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

^ is this not tested in the way the test has been written currently @lzchen ? As I am calling force_flush and shutdown() manually. unless there is something I am not understanding.

Copy link
Member

Choose a reason for hiding this comment

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

They are not called at the same time though, since your call to shutdown() doesn't return until it is done exporting

@codeboten codeboten mentioned this pull request Sep 3, 2020
4 tasks
@codeboten
Copy link
Contributor

Closing this PR, the work needed to ensure that the tracerprovider api is safe to use concurrently is still needed.

@codeboten codeboten closed this Sep 24, 2020
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 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.

5 participants