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: Run _async_no_progress_callback and _async_lag_callback in new t… #1521

Merged
merged 3 commits into from
Oct 12, 2023

Conversation

PatrykGala
Copy link
Contributor

…hread.

Before submitting checklist

  • Did you update the CHANGELOG? (not for test updates, internal changes/refactors or CI/CD setup)
  • Did you ask the docs owner to review all the user-facing changes?

@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Files Coverage Δ
.../operation_processors/async_operation_processor.py 83.59% <0.00%> (ø)
src/neptune/internal/threading/daemon.py 76.84% <0.00%> (-1.29%) ⬇️

... and 106 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@@ -162,7 +162,7 @@ def _check_no_progress(self) -> None:

with self._lock:
if self._should_call_no_progress_callback:
self._async_no_progress_callback()
threading.Thread(target=self._async_no_progress_callback, daemon=True).start()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

daemon=True False?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure that Python doesn't require to call join()? I heard that sometimes it may be needed (it wasn't Python). Stackoverflow says it's fine: https://stackoverflow.com/questions/49912013/python-what-if-we-call-thread-start-and-leave-it-without-join-or-close but just to be sure 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With join the caller will wait and hold the lock indefinitely.

Copy link
Contributor Author

@PatrykGala PatrykGala Oct 12, 2023

Choose a reason for hiding this comment

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

All background jobs have daemon=True, and I think that callbacks shouldn't block the exit of program.

Copy link
Contributor

Choose a reason for hiding this comment

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

All background jobs have daemon=True, and I think that callbacks shouldn't block the exit of program.

We should probably ask someone from Product Team here :<

Copy link
Contributor

Choose a reason for hiding this comment

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

With join the caller will wait and hold the lock indefinitely.

Yeah, I know. I was thinking about joining at the end of a script but I've looked for something like that and it's fine that we're not calling join at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

But let's proceed with this fix. This could be changed anytime.

@@ -162,7 +162,7 @@ def _check_no_progress(self) -> None:

with self._lock:
if self._should_call_no_progress_callback:
self._async_no_progress_callback()
threading.Thread(target=self._async_no_progress_callback, daemon=True).start()
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure that Python doesn't require to call join()? I heard that sometimes it may be needed (it wasn't Python). Stackoverflow says it's fine: https://stackoverflow.com/questions/49912013/python-what-if-we-call-thread-start-and-leave-it-without-join-or-close but just to be sure 😉

@PatrykGala PatrykGala merged commit f2b5689 into master Oct 12, 2023
@PatrykGala PatrykGala deleted the pg/run-callbacks-async branch October 12, 2023 13:07
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