czue / celery-progress

Drop in, configurable, dependency-free progress bars for your Django/Celery applications.
MIT License
473 stars 90 forks source link

Task exceptions appear in multiple places on frontend #44

Closed EJH2 closed 4 years ago

EJH2 commented 4 years ago

As a result of this change in #31, an exception triggered in a task will appear in both the result and progress bar message elements using the default configuration. Running a task with the code

@shared_task(bind=True)
def http_error_task(self, number):
    progress_recorder = ProgressRecorder(self)
    for i in range(number):
        time.sleep(.1)
        progress_recorder.set_progress(i+1, number)
        if i == int(number/2):
            raise StopIteration('We broke it!')
    return random()*1000

produces a progress bar as shown. image It seems counter-intuitive to me to include the exception message in two places, especially since the result makes more sense in the result element than the progress bar area. That's my personal take on it, however. I'd be happy to hear your thoughts on this.

czue commented 4 years ago

Agree this is weird behavior. I'll look into it when testing and before the next release. Thanks!

czue commented 4 years ago

added logic to only append if resultElement is empty in https://github.com/czue/celery-progress/pull/45

czue commented 4 years ago

@EJH2 can you close this once you confirm it's resolved in 0.11?

EJH2 commented 4 years ago

This issue has been fixed for HTTP in 0.12, but it seems the change did not carry over to the websocket version.

czue commented 4 years ago

Oops sorry! I don't actually have a websocket env.

EJH2 commented 4 years ago

I created this project a couple of months ago to test previous changes, but it still holds up well enough today as a development playground.

OmarWKH commented 4 years ago

To reduce inconsistency between http and websocket they could have more code in common. The data code (http, ws) can be a common function. The function could return data.complete so the http client stops polling and the ws client closes the socket.

Better yet. Make CeleryProgressBar a class and CeleryWebSocketProgressBar a subclass that only overrides the connection code. If that sounds good, I have a branch for this and I can make a PR. Although I didn't test it yet.

EJH2 commented 4 years ago

That would make sense, as the Python backend for the websocket is a subclass of the http backend. If you could check this out, I would be unopposed to approving it!

OmarWKH commented 4 years ago

That would make sense, as the Python backend for the websocket is a subclass of the http backend. If you could check this out, I would be unopposed to approving it!

done