Issue 6881042: [Sync] Fix race condition in P2PNotifier with sending notifications

Issue 6881042: [Sync] Fix race condition in P2PNotifier with sending notifications (Closed)

Created:
9 years, 8 months ago by akalin

Modified:
9 years, 7 months ago

Reviewers:
tim (not reviewing), lipalani1

CC:
chromium-reviews, Raghu Simha, ncarter (slow), Paweł Hajdan Jr., tim (not reviewing), idana

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

Description

[Sync] Fix race condition in P2PNotifier with sending notifications Store any notifications sent when not connected and send them on the next connect. This fixes a race condition in the sync integration tests exposed by the new syncer thread. Make sending of notifications not be blocked on successful subscription. Disable a failing test in jingle_unittests. BUG= TEST= Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=82140

Patch Set 1 #

Patch Set 2 : sync to head #

Patch Set 3 : Sync to head, add unit tests #

Total comments: 2

Patch Set 4 : Address comments #

Created: 9 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+193 lines, -36 lines)			Patch
M	chrome/browser/sync/notifier/p2p_notifier.cc	View		2 chunks	+2 lines, -7 lines	0 comments	Download
M	jingle/jingle.gyp	View	1 2	2 chunks	+2 lines, -0 lines	0 comments	Download
M	jingle/notifier/base/proxy_resolving_client_socket_unittest.cc	View		1 chunk	+3 lines, -2 lines	0 comments	Download
M	jingle/notifier/listener/mediator_thread_impl.h	View	1 2 3	3 chunks	+10 lines, -0 lines	0 comments	Download
M	jingle/notifier/listener/mediator_thread_impl.cc	View	1 2	4 chunks	+25 lines, -1 line	0 comments	Download
A	jingle/notifier/listener/mediator_thread_unittest.cc	View	1 2	1 chunk	+123 lines, -0 lines	0 comments	Download
M	jingle/notifier/listener/notification_defines.h	View		1 chunk	+2 lines, -0 lines	0 comments	Download
A	jingle/notifier/listener/notification_defines.cc	View		1 chunk	+13 lines, -0 lines	0 comments	Download
M	jingle/notifier/listener/talk_mediator.h	View		1 chunk	+1 line, -1 line	0 comments	Download
M	jingle/notifier/listener/talk_mediator_impl.h	View		3 chunks	+2 lines, -4 lines	0 comments	Download
M	jingle/notifier/listener/talk_mediator_impl.cc	View		3 chunks	+2 lines, -8 lines	0 comments	Download
M	jingle/notifier/listener/talk_mediator_unittest.cc	View		2 chunks	+8 lines, -13 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

akalin

Synced to head, added unit tests. On 2011/04/19 06:24:57, akalin wrote: > +tim and +lingesh ...

9 years, 8 months ago (2011-04-19 07:36:31 UTC) #2

lipalani1

LGTM with one comment. http://codereview.chromium.org/6881042/diff/1005/jingle/notifier/listener/mediator_thread_impl.cc File jingle/notifier/listener/mediator_thread_impl.cc (right): http://codereview.chromium.org/6881042/diff/1005/jingle/notifier/listener/mediator_thread_impl.cc#newcode300 jingle/notifier/listener/mediator_thread_impl.cc:300: base_task)); It looks like weakpointers ...

9 years, 8 months ago (2011-04-19 19:13:09 UTC) #3

akalin

9 years, 8 months ago (2011-04-19 19:16:40 UTC) #4

Submitting now.

http://codereview.chromium.org/6881042/diff/1005/jingle/notifier/listener/med...
File jingle/notifier/listener/mediator_thread_impl.cc (right):

http://codereview.chromium.org/6881042/diff/1005/jingle/notifier/listener/med...
jingle/notifier/listener/mediator_thread_impl.cc:300: base_task));
On 2011/04/19 19:13:09, lipalani1 wrote:
> It looks like weakpointers are not thread safe. Can you add comments to this
> method explaining how it is the same thread.

Done, added comment to header file.

Expand Messages | Collapse Messages