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

DB change notifications not posted if another thread is in a transaction #2291

Closed
snej opened this issue Dec 21, 2018 · 4 comments

Comments

@snej
Copy link
Member

commented Dec 21, 2018

As mentioned in #2266: -[CBLDatabase postDatabaseChanged] returns without doing anything if the C4Database is in a transaction (on some other thread.) It shouldn't be doing that — LiteCore won't notify it again until it consumes the changes by calling c4dbobs_getChanges, so it will just stop getting change notifications.

I think that test predated multi-thread support, and it made sense then, but it's a bug now. I believe it's safe to just take it out.

@pasin pasin added this to the Iridium milestone Dec 22, 2018

@djpongh djpongh added the ready label Jan 11, 2019

@borrrden

This comment has been minimized.

Copy link
Member

commented Jan 18, 2019

I forgot pretty much everything we talked about when this was discussed, but in the meantime I removed the "InTransaction" check on .NET and the automated tests show no ill effect so that's a plus. Was there something else I needed to check?

@pasin

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

We should also confirm that using inBatch: will not cause multiple notifications.

@borrrden

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

That behavior is already tested on .NET, and I believe on other platforms as well, via the testDatabaseChange test method. It checks that a notification comes through with 10 documents after adding 10 in batch.

@pasin pasin assigned pasin and unassigned borrrden Jan 23, 2019

@pasin

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2019

I can confirm that iOS has the same test and the test still passes after removing the transaction check.

pasin added a commit that referenced this issue Jan 23, 2019

Remove in-transaction check in postDatabaseChanged
We don’t need to check if the database is in transaction before posting the notification as LiteCore will post the change after the transaction is done. Checking in-transaction could also cause missing notification when there is a transaction opened on another thread.

#2291

pasin added a commit that referenced this issue Jan 23, 2019

Remove in-transaction check in postDatabaseChanged (#2342)
We don’t need to check if the database is in transaction before posting the notification as LiteCore will post the change after the transaction is done. Checking in-transaction could also cause missing notification when there is a transaction opened on another thread.

#2291

@pasin pasin closed this Jan 23, 2019

@pasin pasin removed the ready label Jan 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.