Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22702003:
Fix leak of sequenced worker pool in unit tests when service_ is replaced. (Closed)
|
Created:
7 years, 4 months ago by juanlang Modified:
7 years, 4 months ago Reviewers:
Ryan Sleevi CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionFix leak of sequenced worker pool in unit tests when service_ is replaced.
BUG=270192
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217115
Patch Set 1 #Patch Set 2 : More correct fix #
Total comments: 2
Patch Set 3 : Comments improved #
Total comments: 3
Patch Set 4 : More straightforward fix #Patch Set 5 : Flush, just in case, when generation occurs #
Total comments: 1
Patch Set 6 : Use PostTask #Messages
Total messages: 14 (0 generated)
Ryan, how's this look? I ran it through valgrind and it's happy now. I'm not entirely happy with having to manage the pool myself in the unit tests where I replace the service, so I'd appreciate suggestions on how to clean it up further. Apologies for all the n00bness here.
On 2013/08/08 20:53:30, juanlang wrote: > Ryan, how's this look? I ran it through valgrind and it's happy now. I'm not > entirely happy with having to manage the pool myself in the unit tests where I > replace the service, so I'd appreciate suggestions on how to clean it up > further. > > Apologies for all the n00bness here. Can you explain why this is necessary though? The service_ should have no effect on the sequenced_worker_pool_, so you shouldn't need to reset it between runs. I'm trying to figure out what's missing here?
On 2013/08/08 22:00:16, Ryan Sleevi wrote: > On 2013/08/08 20:53:30, juanlang wrote: > > Ryan, how's this look? I ran it through valgrind and it's happy now. I'm not > > entirely happy with having to manage the pool myself in the unit tests where I > > replace the service, so I'd appreciate suggestions on how to clean it up > > further. > > > > Apologies for all the n00bness here. > > Can you explain why this is necessary though? > > The service_ should have no effect on the sequenced_worker_pool_, so you > shouldn't need to reset it between runs. I'm trying to figure out what's missing > here? You're right, this isn't correct. It's a little more subtle, and it's due to my use of a mock. I'll update with a more correct fix in a sec.
On 2013/08/08 23:02:41, juanlang wrote: > On 2013/08/08 22:00:16, Ryan Sleevi wrote: > > On 2013/08/08 20:53:30, juanlang wrote: > > > Ryan, how's this look? I ran it through valgrind and it's happy now. I'm not > > > entirely happy with having to manage the pool myself in the unit tests where > I > > > replace the service, so I'd appreciate suggestions on how to clean it up > > > further. > > > > > > Apologies for all the n00bness here. > > > > Can you explain why this is necessary though? > > > > The service_ should have no effect on the sequenced_worker_pool_, so you > > shouldn't need to reset it between runs. I'm trying to figure out what's > missing > > here? > > You're right, this isn't correct. It's a little more subtle, and it's due to my > use of a mock. I'll update with a more correct fix in a sec. Is this change better?
https://codereview.chromium.org/22702003/diff/8001/net/ssl/server_bound_cert_... File net/ssl/server_bound_cert_service_unittest.cc (right): https://codereview.chromium.org/22702003/diff/8001/net/ssl/server_bound_cert_... net/ssl/server_bound_cert_service_unittest.cc:537: base::MessageLoop::current()->RunUntilIdle(); Consider updating the comments to be more descriptive on why both calls are necessary. See the other tests here.
Thanks, Ryan. https://codereview.chromium.org/22702003/diff/8001/net/ssl/server_bound_cert_... File net/ssl/server_bound_cert_service_unittest.cc (right): https://codereview.chromium.org/22702003/diff/8001/net/ssl/server_bound_cert_... net/ssl/server_bound_cert_service_unittest.cc:537: base::MessageLoop::current()->RunUntilIdle(); On 2013/08/08 23:18:46, Ryan Sleevi wrote: > Consider updating the comments to be more descriptive on why both calls are > necessary. See the other tests here. Done, I think. The rationale is different, and unlike the other tests, I don't need to wait to ensure correct results. I also moved the calls to the end of the tests to make it clearer that they're not necessary for correctness, merely for good memory behavior.
Apologies for being asleep at the wheel with the mock bit - it works, but I think it's the wrong approach. Explanation below. https://codereview.chromium.org/22702003/diff/13001/net/ssl/server_bound_cert... File net/ssl/server_bound_cert_service_unittest.cc (right): https://codereview.chromium.org/22702003/diff/13001/net/ssl/server_bound_cert... net/ssl/server_bound_cert_service_unittest.cc:547: sequenced_worker_pool_->FlushForTesting(); I can't help but keep reading this comment and code and not understanding what's expected/correct. It seems like your Mock is broken/violating expectations. Should you be PostTasking from CallGetServerBoundCertCallbackWithResult to match the semantics of the ServerBoundCertStore, so that you then use FlushForTesting & RunUntilIdle? This definitely seems to match the interface you're mocking, since you're returning ERR_IO_PENDING (which we always handle via PostTask yielding)
https://codereview.chromium.org/22702003/diff/13001/net/ssl/server_bound_cert... File net/ssl/server_bound_cert_service_unittest.cc (right): https://codereview.chromium.org/22702003/diff/13001/net/ssl/server_bound_cert... net/ssl/server_bound_cert_service_unittest.cc:547: sequenced_worker_pool_->FlushForTesting(); On 2013/08/09 00:45:10, Ryan Sleevi wrote: > I can't help but keep reading this comment and code and not understanding what's > expected/correct. Yeah, that big comment should have been a clue to me that I still hadn't understood entirely. These tests are similar to the other tests. I repeat the pattern of the other tests here. > It seems like your Mock is broken/violating expectations. I thought so too, but upon further reading I believe my Mock is behaving substantially like the real store. https://codereview.chromium.org/22702003/diff/18001/net/ssl/server_bound_cert... File net/ssl/server_bound_cert_service_unittest.cc (right): https://codereview.chromium.org/22702003/diff/18001/net/ssl/server_bound_cert... net/ssl/server_bound_cert_service_unittest.cc:571: base::MessageLoop::current()->RunUntilIdle(); But here I believe I don't need to Flush(), because the result should be immediately available. Please correct me if I'm wrong.
On 2013/08/09 04:48:52, juanlang wrote: > https://codereview.chromium.org/22702003/diff/13001/net/ssl/server_bound_cert... > File net/ssl/server_bound_cert_service_unittest.cc (right): > > https://codereview.chromium.org/22702003/diff/13001/net/ssl/server_bound_cert... > net/ssl/server_bound_cert_service_unittest.cc:547: > sequenced_worker_pool_->FlushForTesting(); > On 2013/08/09 00:45:10, Ryan Sleevi wrote: > > I can't help but keep reading this comment and code and not understanding > what's > > expected/correct. > > Yeah, that big comment should have been a clue to me that I still hadn't > understood entirely. These tests are similar to the other tests. I repeat the > pattern of the other tests here. > > > It seems like your Mock is broken/violating expectations. > > I thought so too, but upon further reading I believe my Mock is behaving > substantially like the real store. > > https://codereview.chromium.org/22702003/diff/18001/net/ssl/server_bound_cert... > File net/ssl/server_bound_cert_service_unittest.cc (right): > > https://codereview.chromium.org/22702003/diff/18001/net/ssl/server_bound_cert... > net/ssl/server_bound_cert_service_unittest.cc:571: > base::MessageLoop::current()->RunUntilIdle(); > But here I believe I don't need to Flush(), because the result should be > immediately available. Please correct me if I'm wrong. Answering my own question: the Flush is needed, I'll re-add it in a sec. I believe the existing tests' comments are misleading: the callback.WaitForResult()s are sufficient to get correct behavior, i.e. to wait for certificate generation to occur: the callback won't be ready until the certificate generation is complete. I believe the FlushForTesting()s and RunUntilIdles()s are necessary to ensure memory is released by the time the unit tests complete. I suspect an alternative fix would be to change the SequencedWorkerPool we use here to block on shutdown instead, and remove all the FlushForTesting()/RunUntilIdle()s. I'll play around with these theories this morning.
Thanks for putting me on the right track, Ryan. https://codereview.chromium.org/22702003/diff/13001/net/ssl/server_bound_cert... File net/ssl/server_bound_cert_service_unittest.cc (right): https://codereview.chromium.org/22702003/diff/13001/net/ssl/server_bound_cert... net/ssl/server_bound_cert_service_unittest.cc:547: sequenced_worker_pool_->FlushForTesting(); On 2013/08/09 00:45:10, Ryan Sleevi wrote: > It seems like your Mock is broken/violating expectations. Should you be > PostTasking from CallGetServerBoundCertCallbackWithResult to match the semantics > of the ServerBoundCertStore, so that you then use FlushForTesting & > RunUntilIdle? Yep, PostTask is the right thing here. Thanks. I don't need to FlushForTesting and RunUntilIdle when I use it, either, because by PostTasking and then calling callback.WaitForResult, the workers correctly terminate already. In other words, done.
On 2013/08/09 16:25:28, juanlang wrote: > Thanks for putting me on the right track, Ryan. > > https://codereview.chromium.org/22702003/diff/13001/net/ssl/server_bound_cert... > File net/ssl/server_bound_cert_service_unittest.cc (right): > > https://codereview.chromium.org/22702003/diff/13001/net/ssl/server_bound_cert... > net/ssl/server_bound_cert_service_unittest.cc:547: > sequenced_worker_pool_->FlushForTesting(); > On 2013/08/09 00:45:10, Ryan Sleevi wrote: > > It seems like your Mock is broken/violating expectations. Should you be > > PostTasking from CallGetServerBoundCertCallbackWithResult to match the > semantics > > of the ServerBoundCertStore, so that you then use FlushForTesting & > > RunUntilIdle? > > Yep, PostTask is the right thing here. Thanks. I don't need to FlushForTesting > and RunUntilIdle when I use it, either, because by PostTasking and then calling > callback.WaitForResult, the workers correctly terminate already. > > In other words, done. Ping. I think this is the correct fix now, and I'd like to land it so people don't get too grumpy looking at Valgrind output.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/juanlang@google.com/22702003/23001
Message was sent while issue was closed.
Change committed as 217115 |
