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

Timeouts when deleting a post #821

Closed
pkfrank opened this issue Oct 4, 2018 · 8 comments
Closed

Timeouts when deleting a post #821

pkfrank opened this issue Oct 4, 2018 · 8 comments
Labels
bug always open for contribution external contributors welcome contribution is welcome!

Comments

@pkfrank
Copy link
Contributor

pkfrank commented Oct 4, 2018

Describe the bug
As a user, I want to be able to Delete my posts. Currently, it occasionally times out.

To Reproduce
This seems to happen more often with articles that have a lot of reactions/comments. So, to reproduce, choose an article you're willing to delete with many reactions/comments and press Delete from your dashboard.

Expected behavior
The article is deleted without timeouts.

@jessleenyc jessleenyc added the bug always open for contribution label Oct 4, 2018
@rhymes
Copy link
Contributor

rhymes commented Oct 5, 2018

I believe this is due to this line https://github.com/thepracticaldev/dev.to/blob/master/app/controllers/articles_controller.rb#L128

destroy! calls in a cascade all destroy methods of all related objects that can be deleted. If it's a lot of content it will take some time. Given that deletion is done in process (which means the user has to wait for everything to be deleted during their HTTP request) and given that Heroku times out at 30 seconds, this is a likely reason why big trees of content (the article, plus many comments and many reactions and notifications) can go past the limit.

In addition there's this before_destroy_actions that executes before all of what I mentioned before, I guess this executes synchronously as well.

Can I be assigned to this?

@maestromac
Copy link
Member

Yep @rhymes ! give it a whirl.

@tylermcginnis
Copy link

Getting this as well.

screen shot 2018-12-11 at 8 32 27 am

@Zhao-Andy
Copy link
Contributor

This is still an issue, so raising it up again and adding a few details:

We could make the destroy action in the controller run asynchronously, but I don't think that's ideal. A better solution might be to write a service object whose sole responsibility is to handle destroying articles (and all the dependents).

A few -- but not exhaustive -- list of things we'll need to consider:

  1. Properly purging all caches of the article, comments on the article, and possibly any cached routes (/reactions?article_id=123 for example)
  2. Removing the article and comments from Algolia's index
  3. Handling any errors that might happen

@rhymes
Copy link
Contributor

rhymes commented Jan 8, 2019

Hi @Zhao-Andy I haven't actually looked into it after all, I'm sorry. I don't think it's an easy PR for me because it requires familiarity with the caching system or Algolia and I don't have that.

Maybe you can give a read to the analysis of the bug I wrote back then on dev.to.

There are a services and aspects to consider: caches, fastly, algolia, stream.io and probably others

I think the best solution is to do a two phase removal:

  1. the user needs the article to be gone immediately
  2. everything else (caches, algolia, fastly, stream.io maybe as well, dependent objects in the DB) can be removed out of process in the service object that tracks everything and cleans up

@stale
Copy link

stale bot commented Apr 8, 2019

Thanks for contributing to this issue. As it has been 90 days since the last activity, we are automatically closing the issue in 7 days. This is often because the request was already solved in some way and it just wasn't updated or it's no longer applicable. If this issue still requires attention, please respond with a comment. Happy Coding!

@stale stale bot added the stale label Apr 8, 2019
@rhymes
Copy link
Contributor

rhymes commented Apr 8, 2019

still an issue

@mstruve
Copy link
Contributor

mstruve commented May 12, 2020

We now destroy articles in the background 😄

@mstruve mstruve closed this as completed May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug always open for contribution external contributors welcome contribution is welcome!
Projects
None yet
Development

No branches or pull requests

7 participants