Refactor thread fetching into an iterative operation

[aumetra]

Is your feature request related to a problem? Please describe.

Right now thread fetching is a recursive operation and overflows the stack quite quickly. We already introduced the recursion limit but it already overflows when only fetching 10 replies.

This isn't ideal.

Describe the solution you'd like

Technically every recursive algorithm can be rewritten as an iterative one and vice versa. This is what we need to do here. Make the thread fetching iterative instead.

[tesaguri]

I think the ideal handling of threads is to use an asynchronous job worker like Mastodon's ThreadResolveWorker instead of occupying a task until the thread is fully resolved, iterative or not.

Maybe this would require the database schema to be aware of replies whose in_reply_to_id is yet to be resolved.

[aumetra]

Yeah, good point. But even if it's run in a background task, I'd personally prefer an iterative operation since it won't exhaust the stack space because of function calls

[aumetra]

Maybe this would require the database schema to be aware of replies whose in_reply_to_id is yet to be resolved

Not sure if we need that. We could just set that to NULL if we haven't resolved that and then just enqueue a new job to resolve the thread on initial fetch

[tesaguri]

Yeah, good point. But even if it's run in a background task, I'd personally prefer an iterative operation since it won't exhaust the stack space because of function calls

I was assuming that the worker would only fetch a single post in a job and, well, recursively enqueue another job to the scheduler to fetch the reply until the thread is fully resolved, so that it won't exhaust the stack (and each job is reasonably short-lived). That's how Mastodon's worker is implemented IIUC.

We could just set that to NULL if we haven't resolved that and then just enqueue a new job to resolve the thread on initial fetch

Yes, I think we don't need this if we can hold a transaction over the whole thread resolution process, but otherwise, clients may observe a reply whose in_reply_to post haven't been resolved. Just making the in_reply_to_id to NULL in this case would make the client to recognise the post as a non-reply, which is not true.

Well, come to think of it, we don't keep a transaction over the whole process_new_object function today either, so this problem might not be inherent to the said worker approach anyway.

[aumetra]

To be fair, the whole ActivityPub fetching logic with the database mapping needs a major cleanup. It's in an abysmal state at the moment.

Maybe that cleanup can also add better testing to it finally.