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

Unsupported states crash Progress reporting #54

Open
EJH2 opened this issue Oct 5, 2020 · 10 comments
Open

Unsupported states crash Progress reporting #54

EJH2 opened this issue Oct 5, 2020 · 10 comments
Labels
help wanted Extra attention is needed

Comments

@EJH2
Copy link
Collaborator

EJH2 commented Oct 5, 2020

Currently, there seems to be two task states that will slip past the currently accounted for states in Progress.get_info():

  1. IGNORED
    • HTTP: When used with a progress recorder's stop_task() as demonstrated in Stopping celery task #4, stop_task() will function as expected. However, if stop_task() is not used, upon finishing the task, the result backend will still have the PROGRESS state, meaning the client-side javascript will continue to request the same resource over and over again until the page is closed.
    • WS: As with HTTP, when used with stop_task(), it works fine. Without, the progress bar will get to max but never complete, and as the task has already technically finished, no further updates will be sent. The JS will be left to sit, waiting for a response that will never come, and will wait until the server times out the connection.
  2. RETRY (as noted by Support task retries #53)
    • HTTP: Once the retry exception is thrown, the progress request will try to serialize the error and fail, returning a 500 error for the http request and killing the bar.
    • WS: Once the retry exception is thrown, the progress request will try to serialize the error and fail, forcing the post-run handler to retry sending the error over and over until the retried task is successful.

Both situations pose an interesting challenge on how they'll be handled. For IGNORED, it may be worth revisiting stop_task()'s implementation and offering a possibly better solution than what is currently being used. Successfully fixing the handling for RETRY could eventually pave the way for #13 to get some as love as well. Thoughts on this would be greatly appreciated!

@czue czue added the help wanted Extra attention is needed label Oct 5, 2020
@OmarWKH
Copy link
Collaborator

OmarWKH commented Oct 16, 2020

Note that after #52, unsupported states do not crash celery-progress but do show an error (except ignored tasks).

Regarding IGNORED tasks, do we actually have to do anything about it? It's reasonable that if a task is ignored, then celery-progress can't know its state.

Also, we should test what happens:

@EJH2
Copy link
Collaborator Author

EJH2 commented Oct 19, 2020

For IGNORED tasks, I believe we can somewhat deduce the state of the task. Ignoring a task forces it to finish, thereby marking it as complete. Whether or not it was successful, we do not know, but I was thinking that on the frontend that we fill the bar and grey it out and not return a result, but I am open for suggestions on that as well.

For REVOKED tasks, I was planning on looking at that as a part of the original issue. Unfortunately, I could not test it on my Windows machine, so I omitted it. Now that I am using a Linux server to test, I will definitely investigate that behavior.

I can't seem to find any example usages of store_errors_even_if_ignored, aside from Celery's internal usage. If you could take a look at that, I would be greatly appreciative.

@EJH2 EJH2 mentioned this issue Oct 24, 2020
@OmarWKH
Copy link
Collaborator

OmarWKH commented Oct 25, 2020

For IGNORED tasks, I believe we can somewhat deduce the state of the task. Ignoring a task forces it to finish, thereby marking it as complete. Whether or not it was successful, we do not know, but I was thinking that on the frontend that we fill the bar and grey it out and not return a result, but I am open for suggestions on that as well.

Makes sense. The frontend will need to know which tasks are ignored. Either the user passes an argument when starting the bar, or the value of result.ignored is included in get_info.

I can't seem to find any example usages of store_errors_even_if_ignored, aside from Celery's internal usage. If you could take a look at that, I would be greatly appreciative.

I'll try to get to it soon. If it works as expected, the user should enable it in their own tasks/celery configuration if they want its behavior.

This was referenced Oct 27, 2020
@norbertcyran
Copy link

norbertcyran commented Oct 28, 2020

I'd also like to see ABORTED state being handled correctly.

Ideally, lib should support any user-defined custom state. For example by inheritance, or registering handlers.

For ABORTED case is rather simple. That's an intermediate state, so I believe behavior could be the same as with PROGRESS state.

@EJH2
Copy link
Collaborator Author

EJH2 commented Oct 28, 2020

@norbertcyran I can see about the ABORTED state, seems rather similar to REVOKED in terms of functionality (both will terminate the task) so I could probably just patch it in there. As for user-defined custom states via inheritance or handlers, would you happen to have an example of such?

@norbertcyran
Copy link

@EJH2 ABORTED state doesn't really terminate the task - it just informs the task it should be stopped. But it's left to the task itself where it'll be stopped, example:

@shared_task(bind=True, base=AbortableTask)
def abortable():
    for _ in range(100):
        if self.is_aborted():
            return 
        do_sth()

So I'd expect payload more like:

{
  "completed": false,
  "state": "ABORTED",
  "progress": {...}
}

@EJH2
Copy link
Collaborator Author

EJH2 commented Oct 28, 2020

@norbertcyran All completed really does in the payload is tell the front-end progress bar whether or not it should keep a connection to receive more updates (for HTTP, this means sending a GET request every x interval, for WS this means maintaining the connection). If we pass false with the ABORTED state, that means that the bar will still try to listen for other events, which may or may not be intended behavior. I'm open to your suggestions on how to handle this, though.

@norbertcyran
Copy link

norbertcyran commented Oct 29, 2020

@EJH2 And that's exactly what I expect in my use case. When you call .abort() on task, its state turns to ABORTED until it hits is_aborted() check, after that, following the example I provided, task returns correctly, and its state turns to SUCCESS (unless you raise some exception after is_aborted() check, then it will be FAILURE).
When it comes to frontend, I don't want the connection to be stopped when it gets the ABORTED state, instead, I want it to wait until it turns to SUCCESS or FAILURE. Even if the task is written incorrectly and there's no call to is_aborted(), ABORTED state will change to SUCCESS when the task completes as always

@EJH2
Copy link
Collaborator Author

EJH2 commented Oct 29, 2020

@norbertcyran Understood. Do you believe it would be more appropriate if the progress bar maintained it's current progress from when it was aborted, or should it fill up to max? The bar will fill to max once it hits a SUCCESS or FAILURE state, but we don't really have a precedent set for intermediary states. Still curious on how we would support other user-defined states, though.

@OmarWKH
Copy link
Collaborator

OmarWKH commented Nov 17, 2020

I can't seem to find any example usages of store_errors_even_if_ignored, aside from Celery's internal usage. If you could take a look at that, I would be greatly appreciative.

I defined a task as follows:

@shared_task(ignore_result=True, store_errors_even_if_ignored=True)
def a_task():
   ...

I raised an error inside the task and the FAILURE state was not ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants