[baloo_file_extractor] Be more resistant to multiple QSocketNotifier events
Needs RevisionPublic

Authored by poboiko on Aug 7 2019, 5:56 PM.

Details

Reviewers
bruns
ngraham
Group Reviewers
Baloo
Summary

When a new batch arrives at stdin (as notified by QSocketNotifier), extractor
creates a new transaction. If code, which reacts to notification, gets executed
multiple times, we end up having two write transaction, which is a bad idea.

Current implementation disables QSocketNotifier, so that we won't receive new
notifications. It blocks further notifications, but it does not eliminate the
possibility when several notifications already appeared in the event queue
(which can happen due to race condition).
It shouldn't happen, normally, but the only guard for that case currently is a
single Q_ASSERT, which gets silently ignored in the non-debug build.

This patch replaces assert with proper check, which instead commits & deletes
the transaction if there is any (just in case).
It also removes attempts to open the database multiple times (for each batch),
which is a bad idea, as LMDB documentation suggests. These attempts will be
ignored anyway (because of the check inside Database::open), yet semantically
it's better to move the code outside, to the main(), as it is in baloo_file.

Test Plan

I didn't manage to cause such race condition "in the laboratory environment",
so I've only checked it does not break anything

Diff Detail

Repository
R293 Baloo
Branch
extractor-transaction (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 14848
Build 14866: arc lint + arc unit
poboiko created this revision.Aug 7 2019, 5:56 PM
Restricted Application added projects: Frameworks, Baloo. · View Herald TranscriptAug 7 2019, 5:56 PM
poboiko requested review of this revision.Aug 7 2019, 5:56 PM
ngraham added inline comments.Aug 7 2019, 6:42 PM
src/file/extractor/app.h
50

This isn't public to outside users, right?

broulik added a subscriber: broulik.Aug 7 2019, 9:03 PM
broulik added inline comments.
src/file/extractor/app.h
50

It doens't have an EXPORT macro to it, so I suppose not?

ngraham accepted this revision.Aug 7 2019, 10:13 PM

I can't reproduce the issue either, but this doesn't seem to cause any regressions that I can see, and the code makes sense to me.

This revision is now accepted and ready to land.Aug 7 2019, 10:13 PM
bruns requested changes to this revision.Aug 7 2019, 10:35 PM
This revision now requires changes to proceed.Aug 7 2019, 10:35 PM
anthonyfieroni added inline comments.
src/file/extractor/app.cpp
76–81

processNextFile should be called till m_ids ends, why it does not happen, probably that's why @bruns reject your patch

poboiko added inline comments.Aug 8 2019, 8:31 AM
src/file/extractor/app.cpp
76–81

Well, if that happens, something went wrong, and we have to do something.
As far as I can see, the options are:

  1. Ignore new batch (that would be simply return)
  2. Ignore old batch (that is done now - committing something that was extracted from the old one if there is any)
  3. Try to merge batches and process everything (that would be m_ids.append(new batch) and do not create a new transaction). This might require some additional housekeeping though, as we do not want resulting transaction to be larger than batchSize. And we probably do not want to do our own splitting here - as it would duplicate work done in the parent baloo_file process.

However, since ignored batch do not get removed from ContentIndexingDB, our parent process baloo_file will retry those documents eventually. I believe it should be pretty safe to just drop one of the batches here.

src/file/extractor/app.h
50

It's not. It's only used inside main.cpp of baloo_file_extractor.

bruns added a comment.Aug 8 2019, 12:37 PM

@poboiko - is the problem you describe purely theoretical, or did you actually see it happen?

Multiple triggers of slotNewInput are not a problem, the code flow reaches the m_tr = new Transaction(...) only iff a batch has been read. And batches are only issued when the previous one has been signaled completed with the 'B' batch end marker.

@poboiko - is the problem you describe purely theoretical, or did you actually see it happen?

Multiple triggers of slotNewInput are not a problem, the code flow reaches the m_tr = new Transaction(...) only iff a batch has been read. And batches are only issued when the previous one has been signaled completed with the 'B' batch end marker.

To be completely honest, it is theoretical. I do see it should not happen. Provided everything works as expected.
There were just too many examples (even inside baloo), when critical things didn't work as expected, and got silently ignored due to asserts. I thought it won't hurt getting rid of those, replacing them with proper checks. Even if it's checks of stuff that's impossible.

I did see extractor hanging though, without reliable way to reproduce it, and didn't have time to debug it when it happened.
That's why I've started lurking around extractors code in the first place, looking for stuff like that.