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 passing command errors from the engine to worker functions #394

Merged
merged 3 commits into from
May 24, 2021

Conversation

azawlocki
Copy link
Contributor

@azawlocki azawlocki commented May 21, 2021

Resolves #393

Contains fixes for two separate issues reported by A&C Team:

@azawlocki azawlocki requested a review from a team May 21, 2021 13:58
Comment on lines +538 to +539
item = await command_generator.asend(future_results)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We shouldn't handle errors raised in command_generator.asend() as they include exceptions thrown in the worker function.

Comment on lines +129 to +135
while True:
if self._rescheduled_items:
return True
try:
return await asyncio.wait_for(self.has_new_items(), 1.0)
except asyncio.TimeoutError:
pass
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of waiting indefinitely on potentialy blocking await self.has_new_items(), periodically check if there are self._rescheduled_items.

@azawlocki azawlocki force-pushed the az/process-batches-error-handling branch from 12a6fc2 to 9cbee0f Compare May 24, 2021 08:04
Copy link
Contributor

@shadeofblue shadeofblue left a comment

Choose a reason for hiding this comment

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

lgtm

am I right that the smartqueue fix only applies to the tasks coming from async generators?

@azawlocki azawlocki self-assigned this May 24, 2021
@azawlocki azawlocki requested a review from filipgolem May 24, 2021 09:05
@azawlocki
Copy link
Contributor Author

azawlocki commented May 24, 2021

lgtm

am I right that the smartqueue fix only applies to the tasks coming from async generators?

@shadeofblue Yes, in the sense that if you pass a non-async iterator that blocks for a long time before yielding a task to Executor.submit() or Golem.execute_tasks() then the whole yapapi would stop until that task is yielded -- the present PR does not change that.

@azawlocki azawlocki merged commit 197c569 into master May 24, 2021
@azawlocki azawlocki deleted the az/process-batches-error-handling branch May 24, 2021 13:29
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.

Better error message for nonexistent CommandExecuted attribute access
2 participants