fossar / selfoss

multipurpose rss reader, live stream, mashup, aggregation web application
https://selfoss.aditu.de
GNU General Public License v3.0
2.38k stars 345 forks source link

cliupdate sempahore locker #967

Open alexalouit opened 7 years ago

alexalouit commented 7 years ago

Hi,

I found a issue (in my opinion). Yesterday, my database was in trouble with many connections and simultaneous writings. I noticed that there were ~30 cron processes (cliupdate) running.

I think a semaphore would have to be set up to prevent this.

niol commented 7 years ago

Yes I had done that in #597 but this was not merged.

jtojnar commented 7 years ago

What are the merits of locking, compared to UNIQUE constraint (#921)? At a glance, the constraint seems cleaner but I did not give it a proper thought yet.

alexalouit commented 7 years ago

I think file locker will not do useless query, is upper in application level. Nevertheless, the two systems remain complementary.

This can prevent process and memory bombing from cron zombie/defunct processes.

jtojnar commented 7 years ago

I have originally intended to update the items in the races (hence the UPSERT mention) but since the probability of items changing between the races is low, the queries would indeed be useless. With that in mind, I can see the complementarity; I am not convinced that this is not a premature optimization not worth the added complexity, though.

alexalouit commented 7 years ago

ALTER IGNORE TABLE `items` ADD UNIQUE INDEX (`uid` (150), `source`);

from ~111 619 to ~67 562 entries.

alexalouit commented 7 years ago

Same error this morning, multiple process cliupdate performs many sql queries (which are denied from the unique key).

niol commented 7 years ago

I think that any running update should make another one exit immediately to prevent both sources hammering and rogue processes. There are multiple ways of implementing this, I have proposed using file locks, another way would be to use unix sockets (not portable) or the database. I'll simplify my patch and submit a pull request if it has some chance of getting accepted. Otherwise, this issue should be closed.

jtojnar commented 7 years ago

One might also argue that locking should not be a concern of the application and be done on a higher level:

Personally, I am not yet completely sure which level should this be tackled on but since Tobias rejected the original PR, I will side with the higher level solution.

alexalouit commented 7 years ago

After several months of use, the use of:

did the trick. No more zombie process.

I can submit a pull request, what do you think?

jtojnar commented 7 years ago

@alexalouit Sure, we could use it.

alexalouit commented 7 years ago

see #991

Works perfectly since months (~120 000 items, ~470 sources). I'm not familiar with Selfoss, everything is correct?