Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
107803002:
Consistently suggest ephemeral port for QUIC client (Closed)
|
Created:
7 years ago by jar (doing other things) Modified:
7 years ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionCreate and use a seeded random number generator
(PortSuggester) to suggest what ephemeral source port
should be used when opening a UDP client socket in QUIC.
This should increase the likelihood of 0-RTT connections, even if a strike server is not used across a server cluster.
BUG=326545
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239426
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240782
Patch Set 1 #Patch Set 2 : Add consistency test, and fix unsigned-signed compares #Patch Set 3 : More linty fixes. #Patch Set 4 : more lint #Patch Set 5 : #Patch Set 6 : #
Total comments: 22
Patch Set 7 : Repond to wtc review #
Total comments: 51
Patch Set 8 : Respond to comments by rch and wtc #Patch Set 9 : Added histograms.xml description and a comment #Patch Set 10 : Switch to a smaller seed to avoid using unions #Patch Set 11 : Move a paren... save a 64 bit add #
Total comments: 19
Patch Set 12 : Respond to comments by rch #
Total comments: 2
Patch Set 13 : Removed a now-extraneous cast #Patch Set 14 : Fix DCHECK typo, an update Mocks to be compatible #
Total comments: 2
Patch Set 15 : Pulled together duplicate code in socket_test_util.cc #
Total comments: 4
Patch Set 16 : Disable new assertions in quic_factory_stream.cc #
Total comments: 4
Patch Set 17 : Removed DCHECK and associated code to avoid delaying feature landing #Patch Set 18 : Rebase to TOT, including all connect() error handling #Patch Set 19 : Rebase to TOT #
Total comments: 1
Messages
Total messages: 28 (0 generated)
Patch set 6 LGTM. Please fix the unaligned access of uint64 and int. https://codereview.chromium.org/107803002/diff/60002/net/quic/port_suggester.cc File net/quic/port_suggester.cc (right): https://codereview.chromium.org/107803002/diff/60002/net/quic/port_suggester.... net/quic/port_suggester.cc:18: uint64* start_of_seed = reinterpret_cast<uint64*>(seed_); This cast is not portable because not all processors allow unaligned access. The cast on line 32 has the same problem. A common way to do this properly is to use a union: union { unsigned char bytes[base::kSHA1Length]; uint64 first_uint64; int first_int; } seed_; https://codereview.chromium.org/107803002/diff/60002/net/quic/port_suggester.... net/quic/port_suggester.cc:29: // is made, that each call (probably) provides a new suggestion. Nit: that each call => that call ? https://codereview.chromium.org/107803002/diff/60002/net/quic/port_suggester.... net/quic/port_suggester.cc:37: return (rand % range) + min; If |range| is not a power of 2, this results in a nonuniform distribution. Please confirm that's not a problem. https://codereview.chromium.org/107803002/diff/60002/net/quic/port_suggester.h File net/quic/port_suggester.h (right): https://codereview.chromium.org/107803002/diff/60002/net/quic/port_suggester.... net/quic/port_suggester.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Nit: this should say 2013. And "(c)" is now not needed. Also fix the other new files. https://codereview.chromium.org/107803002/diff/60002/net/quic/port_suggester.... net/quic/port_suggester.h:11: #include "net/base/host_port_pair.h" Nit: you may be able to get by with a forward declaration of HostPortPair. https://codereview.chromium.org/107803002/diff/60002/net/quic/port_suggester.... net/quic/port_suggester.h:26: PortSuggester(const HostPortPair& hostport, uint64 per_profile_randomness); Nit: you should explain whose host and port |hostport| is. Alternatively you can probably just name the parameter |server|. https://codereview.chromium.org/107803002/diff/60002/net/quic/port_suggester.... net/quic/port_suggester.h:28: // Generate a pseudo-random int in the given range. Will (probably) return Nit: please specify whether the range is inclusive. https://codereview.chromium.org/107803002/diff/60002/net/quic/port_suggester_... File net/quic/port_suggester_unittest.cc (right): https://codereview.chromium.org/107803002/diff/60002/net/quic/port_suggester_... net/quic/port_suggester_unittest.cc:7: #include <climits> Nit: what is <climits> for? https://codereview.chromium.org/107803002/diff/60002/net/quic/port_suggester_... net/quic/port_suggester_unittest.cc:32: new PortSuggester(HostPortPair("www.com", 443), entropy_); Nit: I suggest "www.example.com". https://codereview.chromium.org/107803002/diff/60002/net/quic/port_suggester_... net/quic/port_suggester_unittest.cc:35: // Use a min an max for the range of |port|. an => and? https://codereview.chromium.org/107803002/diff/60002/net/quic/quic_stream_fac... File net/quic/quic_stream_factory.h (right): https://codereview.chromium.org/107803002/diff/60002/net/quic/quic_stream_fac... net/quic/quic_stream_factory.h:234: // connections to the same server. This comment is confusing because it describes a port suggester. It is more important for this comment to describe the port_entropy_ member, which deals with user profile isolation.
https://chromiumcodereview.appspot.com/107803002/diff/60002/net/quic/port_sug... File net/quic/port_suggester.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/60002/net/quic/port_sug... net/quic/port_suggester.cc:18: uint64* start_of_seed = reinterpret_cast<uint64*>(seed_); On 2013/12/06 23:36:44, wtc wrote: > > This cast is not portable because not all processors allow unaligned access. The > cast on line 32 has the same problem. > > A common way to do this properly is to use a union: > > union { > unsigned char bytes[base::kSHA1Length]; > uint64 first_uint64; > int first_int; > } seed_; Done. https://chromiumcodereview.appspot.com/107803002/diff/60002/net/quic/port_sug... net/quic/port_suggester.cc:29: // is made, that each call (probably) provides a new suggestion. On 2013/12/06 23:36:44, wtc wrote: > > Nit: that each call => that call ? Adjusted sentence. https://chromiumcodereview.appspot.com/107803002/diff/60002/net/quic/port_sug... net/quic/port_suggester.cc:37: return (rand % range) + min; On 2013/12/06 23:36:44, wtc wrote: > > If |range| is not a power of 2, this results in a nonuniform distribution. > Please confirm that's not a problem. Added comment. https://chromiumcodereview.appspot.com/107803002/diff/60002/net/quic/port_sug... File net/quic/port_suggester.h (right): https://chromiumcodereview.appspot.com/107803002/diff/60002/net/quic/port_sug... net/quic/port_suggester.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/12/06 23:36:44, wtc wrote: > > Nit: this should say 2013. And "(c)" is now not needed. > > Also fix the other new files. Done. https://chromiumcodereview.appspot.com/107803002/diff/60002/net/quic/port_sug... net/quic/port_suggester.h:11: #include "net/base/host_port_pair.h" On 2013/12/06 23:36:44, wtc wrote: > > Nit: you may be able to get by with a forward declaration of HostPortPair. Done. https://chromiumcodereview.appspot.com/107803002/diff/60002/net/quic/port_sug... net/quic/port_suggester.h:26: PortSuggester(const HostPortPair& hostport, uint64 per_profile_randomness); On 2013/12/06 23:36:44, wtc wrote: > > Nit: you should explain whose host and port |hostport| is. Alternatively you can > probably just name the parameter |server|. Done (switched to |server|) https://chromiumcodereview.appspot.com/107803002/diff/60002/net/quic/port_sug... net/quic/port_suggester.h:28: // Generate a pseudo-random int in the given range. Will (probably) return On 2013/12/06 23:36:44, wtc wrote: > > Nit: please specify whether the range is inclusive. Done. https://chromiumcodereview.appspot.com/107803002/diff/60002/net/quic/port_sug... File net/quic/port_suggester_unittest.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/60002/net/quic/port_sug... net/quic/port_suggester_unittest.cc:7: #include <climits> On 2013/12/06 23:36:44, wtc wrote: > > Nit: what is <climits> for? <oops> I was using MAX_INT, but switched to using the shared max/min ephemeral_ variables. Removed include (thanks!!). https://chromiumcodereview.appspot.com/107803002/diff/60002/net/quic/port_sug... net/quic/port_suggester_unittest.cc:32: new PortSuggester(HostPortPair("www.com", 443), entropy_); On 2013/12/06 23:36:44, wtc wrote: > > Nit: I suggest "www.example.com". Done. https://chromiumcodereview.appspot.com/107803002/diff/60002/net/quic/port_sug... net/quic/port_suggester_unittest.cc:35: // Use a min an max for the range of |port|. On 2013/12/06 23:36:44, wtc wrote: > > an => and? Improved comment. https://chromiumcodereview.appspot.com/107803002/diff/60002/net/quic/quic_str... File net/quic/quic_stream_factory.h (right): https://chromiumcodereview.appspot.com/107803002/diff/60002/net/quic/quic_str... net/quic/quic_stream_factory.h:234: // connections to the same server. On 2013/12/06 23:36:44, wtc wrote: > > This comment is confusing because it describes a port suggester. It is more > important for this comment to describe the port_entropy_ member, which deals > with user profile isolation. Extended comment. See if this is clearer.
Patch set 7 LGTM. https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... File net/quic/port_suggester.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... net/quic/port_suggester.cc:28: // Sometimes our suggestion can't be used, so we ensure that if a additional Nit: remove "a" https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... net/quic/port_suggester.cc:29: // calls are made, that each call (probably) provides a new suggestion. Nit: I think "that" can be removed https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... File net/quic/port_suggester.h (right): https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... net/quic/port_suggester.h:42: int first_int; The indentation of these four lines seem off by one. https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... File net/quic/port_suggester_unittest.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... net/quic/port_suggester_unittest.cc:91: new PortSuggester(HostPortPair("www.example.ORG", 80), entropy_), Nit: just curious why you capitalize "org"? https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/quic_st... File net/quic/quic_stream_factory.h (right): https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/quic_st... net/quic/quic_stream_factory.h:237: // profiles (probabalistically) prevent two profiles from colliding in their probabalistically => probablistically
Thanks for doing this! It'll be great to see the impact on 0-RTT, before we have strike registers. https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... File net/quic/port_suggester.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... net/quic/port_suggester.cc:19: DCHECK_LE(sizeof(seed_.first_uint64), sizeof(seed_.hash_bytes)); Can you use COMPILE_ASSERT here? https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... net/quic/port_suggester.cc:20: seed_.first_uint64 ^= per_profile_randomness ^ server.port(); Since you've already gone through the trouble of using a sha1 hash, I'm surprised to see the use of XOR here. Did you consider including all this data as initial input to the sha1 hash? https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... net/quic/port_suggester.cc:35: int rand = std::abs(seed_.first_int); I notice that you're using first_int here, and first_uint64 is never use after the constructor? If seed_ were simply a uint64, I think you could get rid of this abs step, and just operate on seed_ directly: int range = max - min + 1; return (seed_ % range) + min; Since seed is unsigned, seed_ % range will be unsigned, and in the right range. https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... File net/quic/port_suggester.h (right): https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... net/quic/port_suggester.h:24: : public base::RefCountedThreadSafe<PortSuggester> { Does this really need to be thread safe? Almost nothing in net/ is thread-safe, so that makes me think this does not need to be. https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... net/quic/port_suggester.h:28: PortSuggester(const HostPortPair& server, uint64 per_profile_randomness); nit: can per_profile_randomness simply be renamed "seed". This class knows nothing about profiles, it's really just using this to help seed the generator, right? https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... net/quic/port_suggester.h:43: } seed_; This union seems really subtle and not terribly readable. Did you consider simply having a unit64 seed_? When you evolve the seed you could then do: unsigned char hash[base::kSHA1Length]; base::SHA1HashBytes(reinterpret_cast<unsigned char*>(&seed_), sizeof(seed_), hash); memcpy(&seed_, hash, sizeof(seed_)); (This would make your seed_ member variable 8 bytes, instead of 20. A whopping 12 byte savings :>) https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... File net/quic/port_suggester_unittest.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... net/quic/port_suggester_unittest.cc:47: int insertion_limit = 200; // We should be done by then. nit: if port_range is const, should insertion limit be const, too? https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... net/quic/port_suggester_unittest.cc:51: if (ports.size() == static_cast<size_t>(port_range)) { instead of casting, how about making port_range a size_t? https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... net/quic/port_suggester_unittest.cc:59: // When the range is large, duplicates are rare, but we'll ask for a few There have been a few threads on chromium-dev about making sure that "random" tests don't occasionally fail. It looks like these tests are fine, because you're seeding with a fixed constant, the behavior is totally consistent. Is that right? https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... net/quic/port_suggester_unittest.cc:69: EXPECT_EQ(static_cast<size_t>(port_count), ports.size()); ditto about the cast https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... net/quic/port_suggester_unittest.cc:87: TEST_F(PortSuggesterTest, DifferentHostPorts) { nit: Since you also vary the entropy in the tests, should you add Entropy to the test name? https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/quic_st... File net/quic/quic_stream_factory.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/quic_st... net/quic/quic_stream_factory.cc:457: base::Bind(&PortSuggester::SuggestPort, port_suggester), It would be super great if we had a test that verified we actually use the same port when connecting to the same server. Alas, that doesn't look trivial. :/ It's tempting to add a unit test of QuicStreamFactory, but that use the MockSocket infrastructure, which doesn't call the rand_cb. You could probably tweak a test to use a real socket factory, but that'd be a a bunch of awkward. Probably the easiest thing would be to tweak the QuicServer to expose the peer_address in some fashion, but even that's a bunch of yak-shaving, so perhaps it's not worth it.
https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester.cc File net/quic/port_suggester.cc (right): https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester... net/quic/port_suggester.cc:19: DCHECK_LE(sizeof(seed_.first_uint64), sizeof(seed_.hash_bytes)); On 2013/12/07 01:46:14, Ryan Hamilton wrote: > Can you use COMPILE_ASSERT here? Done. https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester... net/quic/port_suggester.cc:20: seed_.first_uint64 ^= per_profile_randomness ^ server.port(); On 2013/12/07 01:46:14, Ryan Hamilton wrote: > Since you've already gone through the trouble of using a sha1 hash, I'm > surprised to see the use of XOR here. Did you consider including all this data > as initial input to the sha1 hash? The only item that should not be (close to) uniformly random is server.port(). As a result, I think I get no reduction in the randomness of the result by just XORing in that one item. https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester... net/quic/port_suggester.cc:28: // Sometimes our suggestion can't be used, so we ensure that if a additional On 2013/12/07 01:03:38, wtc wrote: > > Nit: remove "a" Done. https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester... net/quic/port_suggester.cc:29: // calls are made, that each call (probably) provides a new suggestion. On 2013/12/07 01:03:38, wtc wrote: > > Nit: I think "that" can be removed Done. https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester... net/quic/port_suggester.cc:35: int rand = std::abs(seed_.first_int); On 2013/12/07 01:46:14, Ryan Hamilton wrote: > I notice that you're using first_int here, and first_uint64 is never use after > the constructor? If seed_ were simply a uint64, I think you could get rid of > this abs step, and just operate on seed_ directly: > > int range = max - min + 1; > return (seed_ % range) + min; > > Since seed is unsigned, seed_ % range will be unsigned, and in the right range. Seed_ is currently large enough to hold all the return hash bits... which makes the calls a bit cleaner... and avoids doing any stack allocation (declaration) and memcopy (to truncate). My expectation was that we didn't need to do any 64 bit math, as the port range was generally 16 bits, and we pulled out 32 bits to do the modulo operation. My guess is that Android will actually want to further simplify this calculation (even with 32 bits, let alone 64), as the ARM seems averse to divisions... so I figured it was nicer to leave this simple. If you really want it, I can change it in any direction... but I doubt it will be simpler. https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester.h File net/quic/port_suggester.h (right): https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester... net/quic/port_suggester.h:24: : public base::RefCountedThreadSafe<PortSuggester> { On 2013/12/07 01:46:14, Ryan Hamilton wrote: > Does this really need to be thread safe? Almost nothing in net/ is thread-safe, > so that makes me think this does not need to be. Done. https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester... net/quic/port_suggester.h:28: PortSuggester(const HostPortPair& server, uint64 per_profile_randomness); On 2013/12/07 01:46:14, Ryan Hamilton wrote: > nit: can per_profile_randomness simply be renamed "seed". This class knows > nothing about profiles, it's really just using this to help seed the generator, > right? Done. https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester... net/quic/port_suggester.h:42: int first_int; On 2013/12/07 01:03:38, wtc wrote: > > The indentation of these four lines seem off by one. Done. https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester... net/quic/port_suggester.h:43: } seed_; On 2013/12/07 01:46:14, Ryan Hamilton wrote: > This union seems really subtle and not terribly readable. Did you consider > simply having a unit64 seed_? When you evolve the seed you could then do: > > unsigned char hash[base::kSHA1Length]; > base::SHA1HashBytes(reinterpret_cast<unsigned char*>(&seed_), sizeof(seed_), > hash); > memcpy(&seed_, hash, sizeof(seed_)); > > (This would make your seed_ member variable 8 bytes, instead of 20. A whopping > 12 byte savings :>) I originally used reinterpret casts, but wtc noted that care was needed to avoid alignment issues on some platform... so the union ended up being cleaner. https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester... File net/quic/port_suggester_unittest.cc (right): https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester... net/quic/port_suggester_unittest.cc:47: int insertion_limit = 200; // We should be done by then. On 2013/12/07 01:46:14, Ryan Hamilton wrote: > nit: if port_range is const, should insertion limit be const, too? Done. https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester... net/quic/port_suggester_unittest.cc:51: if (ports.size() == static_cast<size_t>(port_range)) { On 2013/12/07 01:46:14, Ryan Hamilton wrote: > instead of casting, how about making port_range a size_t? ...then I'd have to cast when I pass the range into the SuggestPort() function, that takes two ints :-/. https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester... net/quic/port_suggester_unittest.cc:59: // When the range is large, duplicates are rare, but we'll ask for a few On 2013/12/07 01:46:14, Ryan Hamilton wrote: > There have been a few threads on chromium-dev about making sure that "random" > tests don't occasionally fail. It looks like these tests are fine, because > you're seeding with a fixed constant, the behavior is totally consistent. Is > that right? Exactly. All values and algorithms are fixed... It can fail only if the algorithms changed a lot... and since we're using standards to hash... than it unlikely. These tests just show that we're not doing anything dumb, ignoring args, not calling a hash function, etc. https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester... net/quic/port_suggester_unittest.cc:69: EXPECT_EQ(static_cast<size_t>(port_count), ports.size()); On 2013/12/07 01:46:14, Ryan Hamilton wrote: > ditto about the cast Done. https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester... net/quic/port_suggester_unittest.cc:87: TEST_F(PortSuggesterTest, DifferentHostPorts) { On 2013/12/07 01:46:14, Ryan Hamilton wrote: > nit: Since you also vary the entropy in the tests, should you add Entropy to the > test name? Done. https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester... net/quic/port_suggester_unittest.cc:91: new PortSuggester(HostPortPair("www.example.ORG", 80), entropy_), On 2013/12/07 01:03:38, wtc wrote: > > Nit: just curious why you capitalize "org"? To make it easy to see that was the difference from the first line. https://codereview.chromium.org/107803002/diff/100001/net/quic/quic_stream_fa... File net/quic/quic_stream_factory.cc (right): https://codereview.chromium.org/107803002/diff/100001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:457: base::Bind(&PortSuggester::SuggestPort, port_suggester), On 2013/12/07 01:46:14, Ryan Hamilton wrote: > It would be super great if we had a test that verified we actually use the same > port when connecting to the same server. Alas, that doesn't look trivial. :/ > It's tempting to add a unit test of QuicStreamFactory, but that use the > MockSocket infrastructure, which doesn't call the rand_cb. You could probably > tweak a test to use a real socket factory, but that'd be a a bunch of awkward. > Probably the easiest thing would be to tweak the QuicServer to expose the > peer_address in some fashion, but even that's a bunch of yak-shaving, so perhaps > it's not worth it. I added some code to DCHECK() that the port_suggester was at least being called (recall that there was a bug previously where that didn't happen), and also to verify that the last suggestion was actually used (I'm counting on not being blocked from using the suggestions). The only missing link would be if we're accidentally calling to seed_ differently when we are going to the same server (since the unit tests verify that if you seed identically... you get identical suggestions). https://codereview.chromium.org/107803002/diff/100001/net/quic/quic_stream_fa... File net/quic/quic_stream_factory.h (right): https://codereview.chromium.org/107803002/diff/100001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.h:237: // profiles (probabalistically) prevent two profiles from colliding in their On 2013/12/07 01:03:38, wtc wrote: > > probabalistically => probablistically Done.
https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester.cc File net/quic/port_suggester.cc (right): https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester... net/quic/port_suggester.cc:20: seed_.first_uint64 ^= per_profile_randomness ^ server.port(); On 2013/12/07 03:12:18, jar wrote: > On 2013/12/07 01:46:14, Ryan Hamilton wrote: > > Since you've already gone through the trouble of using a sha1 hash, I'm > > surprised to see the use of XOR here. Did you consider including all this > data > > as initial input to the sha1 hash? > > The only item that should not be (close to) uniformly random is server.port(). > As a result, I think I get no reduction in the randomness of the result by just > XORing in that one item. Ah, I see. Makes sense. https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester... net/quic/port_suggester.cc:35: int rand = std::abs(seed_.first_int); On 2013/12/07 03:12:18, jar wrote: > On 2013/12/07 01:46:14, Ryan Hamilton wrote: > > I notice that you're using first_int here, and first_uint64 is never use after > > the constructor? If seed_ were simply a uint64, I think you could get rid of > > this abs step, and just operate on seed_ directly: > > > > int range = max - min + 1; > > return (seed_ % range) + min; > > > > Since seed is unsigned, seed_ % range will be unsigned, and in the right > range. > > Seed_ is currently large enough to hold all the return hash bits... which makes > the calls a bit cleaner... and avoids doing any stack allocation (declaration) > and memcopy (to truncate). Understood. However, in my opinion the extra computational overhead does not seem to merit the decrease in readability. > My expectation was that we didn't need to do any 64 bit math, as the port range > was generally 16 bits, and we pulled out 32 bits to do the modulo operation. > > My guess is that Android will actually want to further simplify this calculation > (even with 32 bits, let alone 64), as the ARM seems averse to divisions... so I > figured it was nicer to leave this simple. If android and arm are averse to 64 bit math, then they're going to have a conniption fit with QUIC :> We do all sorts for 64 bit math on every packet. Recall that we do a bunch of 64 bit arithmetic to figure out the correct high bits of the sequence number for example. We do this on every *packet*. The port suggestion happens typically once per connection. > If you really want it, I can change it in any direction... but I doubt it will > be simpler. Yes, I think it is clearer. It will remove one of the fields of the union (I think we can get rid of the other, too, see other comments :>) and it will remove the call to abs. This is more readable to me. https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester.h File net/quic/port_suggester.h (right): https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester... net/quic/port_suggester.h:43: } seed_; On 2013/12/07 03:12:18, jar wrote: > On 2013/12/07 01:46:14, Ryan Hamilton wrote: > > This union seems really subtle and not terribly readable. Did you consider > > simply having a unit64 seed_? When you evolve the seed you could then do: > > > > unsigned char hash[base::kSHA1Length]; > > base::SHA1HashBytes(reinterpret_cast<unsigned char*>(&seed_), sizeof(seed_), > > hash); > > memcpy(&seed_, hash, sizeof(seed_)); > > > > (This would make your seed_ member variable 8 bytes, instead of 20. A > whopping > > 12 byte savings :>) > > I originally used reinterpret casts, but wtc noted that care was needed to avoid > alignment issues on some platform... so the union ended up being cleaner. Hm. I don't think there are alignment issues with a char*. Think about it this way. I could have: char bytes[1024]; I should be totally able to say: int i = rand_int_between(0,1024); SHA1HashBytes(&bytes[i], 1, output); Given this, it's clear that alignment can't really be a concern. In any case, SHA1HashBytes calls down to this code: void SecureHashAlgorithm::Update(const void* data, size_t nbytes) { const uint8* d = reinterpret_cast<const uint8*>(data); Notice that it takes a void* and immediately casts it to a char* (well, uint8, but whatever :>). If alignment were a concern, then that would be totally unsafe. Now, if you were going the other way, there could be problems: reinterpret_cast<uint64*>(&bytes[i]) That's not guaranteed to align, so we need to memcpy in this case, as per the last line in my snippet. https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester... File net/quic/port_suggester_unittest.cc (right): https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester... net/quic/port_suggester_unittest.cc:51: if (ports.size() == static_cast<size_t>(port_range)) { On 2013/12/07 03:12:18, jar wrote: > On 2013/12/07 01:46:14, Ryan Hamilton wrote: > > instead of casting, how about making port_range a size_t? > > ...then I'd have to cast when I pass the range into the SuggestPort() function, > that takes two ints :-/. Are you sure that's right? In my experience it's not a problem to pass in unsigned values to signed functions. The problem comes when you *compare* signed to unsigned. Can you switch and see what the compiler says? (We hit this a lot in QUIC, and I *thought* that I understood the rules, but who knows! :>) https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester... net/quic/port_suggester_unittest.cc:59: // When the range is large, duplicates are rare, but we'll ask for a few On 2013/12/07 03:12:18, jar wrote: > On 2013/12/07 01:46:14, Ryan Hamilton wrote: > > There have been a few threads on chromium-dev about making sure that "random" > > tests don't occasionally fail. It looks like these tests are fine, because > > you're seeding with a fixed constant, the behavior is totally consistent. Is > > that right? > > Exactly. All values and algorithms are fixed... Great! https://codereview.chromium.org/107803002/diff/100001/net/quic/quic_stream_fa... File net/quic/quic_stream_factory.cc (right): https://codereview.chromium.org/107803002/diff/100001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:457: base::Bind(&PortSuggester::SuggestPort, port_suggester), On 2013/12/07 03:12:18, jar wrote: > On 2013/12/07 01:46:14, Ryan Hamilton wrote: > > It would be super great if we had a test that verified we actually use the > same > > port when connecting to the same server. Alas, that doesn't look trivial. :/ > > It's tempting to add a unit test of QuicStreamFactory, but that use the > > MockSocket infrastructure, which doesn't call the rand_cb. You could probably > > tweak a test to use a real socket factory, but that'd be a a bunch of awkward. > > > Probably the easiest thing would be to tweak the QuicServer to expose the > > peer_address in some fashion, but even that's a bunch of yak-shaving, so > perhaps > > it's not worth it. > > I added some code to DCHECK() that the port_suggester was at least being called > (recall that there was a bug previously where that didn't happen), and also to > verify that the last suggestion was actually used (I'm counting on not being > blocked from using the suggestions). > > The only missing link would be if we're accidentally calling to seed_ > differently when we are going to the same server (since the unit tests verify > that if you seed identically... you get identical suggestions). If you reverted the changes to this file, would any tests break?
Ryan: Per your request, I switched to using uint64 as a seed_. You can compare the code to patch set 9. I personally prefer patch set 9, but it is not that big a deal either way. Please feel free to hit the CQ button if you're satisfied. https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester.h File net/quic/port_suggester.h (right): https://codereview.chromium.org/107803002/diff/100001/net/quic/port_suggester... net/quic/port_suggester.h:43: } seed_; On 2013/12/07 04:52:22, Ryan Hamilton wrote: > Hm. I don't think there are alignment issues with a char*. Think about it this > way. I could have: > > char bytes[1024]; > > I should be totally able to say: > > int i = rand_int_between(0,1024); > SHA1HashBytes(&bytes[i], 1, output); > > Given this, it's clear that alignment can't really be a concern. In any case, > SHA1HashBytes calls down to this code: > > void SecureHashAlgorithm::Update(const void* data, size_t nbytes) { > const uint8* d = reinterpret_cast<const uint8*>(data); > > Notice that it takes a void* and immediately casts it to a char* (well, uint8, > but whatever :>). If alignment were a concern, then that would be totally > unsafe. > > Now, if you were going the other way, there could be problems: > > reinterpret_cast<uint64*>(&bytes[i]) > > That's not guaranteed to align, so we need to memcpy in this case, as per the > last line in my snippet. The alignment issue (see wtc comments in patch 6) was that char bytes[8]; inside the class is not guaranteed to be any more aligned than a char. As a result, when I try to XOR onto its bytes, or try to extract the first int by casting using: reinterpret_cast<int*>(bytes) ...there would be alignment trouble (potentially). The use of unions avoids all this. I used to think union was just cuter... but I'm now convinced (by wtc) that it also avoids the mis-aligned problems, and I've seen it in tons of code.
Thanks! Looks much more readable to me. https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... File net/quic/port_suggester.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... net/quic/port_suggester.cc:19: DCHECK_LE(sizeof(seed_.first_uint64), sizeof(seed_.hash_bytes)); On 2013/12/07 03:12:18, jar wrote: > On 2013/12/07 01:46:14, Ryan Hamilton wrote: > > Can you use COMPILE_ASSERT here? > > Done. It looks like this check has been removed. I'm totally fine with that, but if you wished, you could do something like: COMPILE_ASSERT(sizeof(seed_) < base::kSHA1Length); https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... File net/quic/port_suggester.h (right): https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... net/quic/port_suggester.h:28: PortSuggester(const HostPortPair& server, uint64 per_profile_randomness); On 2013/12/07 03:12:18, jar wrote: > On 2013/12/07 01:46:14, Ryan Hamilton wrote: > > nit: can per_profile_randomness simply be renamed "seed". This class knows > > nothing about profiles, it's really just using this to help seed the > generator, > > right? > > Done. Looks like it's still called per_profile_randomness. Please rename it to seed. https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... net/quic/port_suggester.h:43: } seed_; On 2013/12/07 21:09:28, jar wrote: > On 2013/12/07 04:52:22, Ryan Hamilton wrote: > > Hm. I don't think there are alignment issues with a char*. Think about it > this > > way. I could have: > > > > char bytes[1024]; > > > > I should be totally able to say: > > > > int i = rand_int_between(0,1024); > > SHA1HashBytes(&bytes[i], 1, output); > > > > Given this, it's clear that alignment can't really be a concern. In any case, > > SHA1HashBytes calls down to this code: > > > > void SecureHashAlgorithm::Update(const void* data, size_t nbytes) { > > const uint8* d = reinterpret_cast<const uint8*>(data); > > > > Notice that it takes a void* and immediately casts it to a char* (well, uint8, > > but whatever :>). If alignment were a concern, then that would be totally > > unsafe. > > > > Now, if you were going the other way, there could be problems: > > > > reinterpret_cast<uint64*>(&bytes[i]) > > > > That's not guaranteed to align, so we need to memcpy in this case, as per the > > last line in my snippet. > > The alignment issue (see wtc comments in patch 6) was that > char bytes[8]; > inside the class is not guaranteed to be any more aligned than a char. As a > result, when I try to XOR onto its bytes, or try to extract the first int by > casting using: > > reinterpret_cast<int*>(bytes) > > ...there would be alignment trouble (potentially). > > The use of unions avoids all this. I used to think union was just cuter... but > I'm now convinced (by wtc) that it also avoids the mis-aligned problems, and > I've seen it in tons of code. Ah, yes, using reinterpret_cast to go from bytes to some wider data type has definite issues. Thankfully, the other direction is not a problem so the code I recommended (and that you have in your CL now) is safe. Yes, the union does align things to ensure safety, but since it took the analysis of an expert to become convinced of that fact, I think it's more readable to avoid it. https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... File net/quic/port_suggester_unittest.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... net/quic/port_suggester_unittest.cc:51: if (ports.size() == static_cast<size_t>(port_range)) { On 2013/12/07 04:52:22, Ryan Hamilton wrote: > On 2013/12/07 03:12:18, jar wrote: > > On 2013/12/07 01:46:14, Ryan Hamilton wrote: > > > instead of casting, how about making port_range a size_t? > > > > ...then I'd have to cast when I pass the range into the SuggestPort() > function, > > that takes two ints :-/. > > Are you sure that's right? In my experience it's not a problem to pass in > unsigned values to signed functions. The problem comes when you *compare* > signed to unsigned. Can you switch and see what the compiler says? (We hit > this a lot in QUIC, and I *thought* that I understood the rules, but who knows! > :>) Ping. https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/port_su... File net/quic/port_suggester.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/port_su... net/quic/port_suggester.cc:40: previous_suggestion_ = static_cast<int>(seed_ % range) + min; I'm not sure that I understand the need for previous_suggestion_. Can you simply do: return (seed_ % range) + min; This avoids the need for previous_suggestion_ at all? edit: Oh, I see, it's used in the DCHECK in quic_stream_factory.cc. nit: If you decide to keep the unit test, you might consider implementing previous_suggestion() as: return (seed_ % range) + min; instead of dedicating a member variable to this function which is only used in a DCHECK (and hence not in the production binary). You could return previous_suggestion(); as the last line from SuggestPort to reuse the code. https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/port_su... File net/quic/port_suggester.h (right): https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/port_su... net/quic/port_suggester.h:23: class NET_EXPORT PortSuggester Should this be NET_EXPORT or NET_EXPORT_PRIVATE? If you do not intend to use it outside of net/, I think you can make it PRIVATE. https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/port_su... net/quic/port_suggester.h:24: : public base::RefCounted<PortSuggester> { nit: I think this can all fit on a single line. (Unless you make it private :>) https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/port_su... net/quic/port_suggester.h:34: int previous_suggestion() const { return previous_suggestion_; } if you decide to keep this method, should it DCHECK_LT(0u, call_count_)? https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/port_su... net/quic/port_suggester.h:42: uint64 seed_; nit: extra space before seed_; https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/port_su... File net/quic/port_suggester_unittest.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/port_su... net/quic/port_suggester_unittest.cc:19: : entropy_(1345689), Out of curiosity, did the previous seed produce problematic results? https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/quic_st... File net/quic/quic_stream_factory.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/quic_st... net/quic/quic_stream_factory.cc:464: address.port() == port_suggester->previous_suggestion()); I don't think that this DCHECK is guaranteed to succeed, is it? If we get *really* unlikely the kBindRetries ports suggested by the suggester may all be taken. As I understand it, at that point we do a default bind (with the suggestion of 0). for (int i = 0; i < kBindRetries; ++i) { int rv = DoBind(IPEndPoint(ip, rand_int_cb_.Run(kPortStart, kPortEnd))); if (rv == OK || rv != ERR_ADDRESS_IN_USE) return rv; } return DoBind(IPEndPoint(ip, 0)); Given this, I think you should remove this DCHECK (and possibly previous_suggestion(), since it would then become unused)
https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... File net/quic/port_suggester.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... net/quic/port_suggester.cc:19: DCHECK_LE(sizeof(seed_.first_uint64), sizeof(seed_.hash_bytes)); On 2013/12/07 23:21:54, Ryan Hamilton wrote: > On 2013/12/07 03:12:18, jar wrote: > > On 2013/12/07 01:46:14, Ryan Hamilton wrote: > > > Can you use COMPILE_ASSERT here? > > > > Done. > > It looks like this check has been removed. I'm totally fine with that, but if > you wished, you could do something like: > > COMPILE_ASSERT(sizeof(seed_) < base::kSHA1Length); Done. https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... File net/quic/port_suggester.h (right): https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... net/quic/port_suggester.h:28: PortSuggester(const HostPortPair& server, uint64 per_profile_randomness); On 2013/12/07 23:21:54, Ryan Hamilton wrote: > On 2013/12/07 03:12:18, jar wrote: > > On 2013/12/07 01:46:14, Ryan Hamilton wrote: > > > nit: can per_profile_randomness simply be renamed "seed". This class knows > > > nothing about profiles, it's really just using this to help seed the > > generator, > > > right? > > > > Done. > > Looks like it's still called per_profile_randomness. Please rename it to seed. My mistake. I thought I did it. DONE now. https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... File net/quic/port_suggester_unittest.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... net/quic/port_suggester_unittest.cc:51: if (ports.size() == static_cast<size_t>(port_range)) { On 2013/12/07 23:21:54, Ryan Hamilton wrote: > On 2013/12/07 04:52:22, Ryan Hamilton wrote: > > On 2013/12/07 03:12:18, jar wrote: > > > On 2013/12/07 01:46:14, Ryan Hamilton wrote: > > > > instead of casting, how about making port_range a size_t? > > > > > > ...then I'd have to cast when I pass the range into the SuggestPort() > > function, > > > that takes two ints :-/. > > > > Are you sure that's right? In my experience it's not a problem to pass in > > unsigned values to signed functions. The problem comes when you *compare* > > signed to unsigned. Can you switch and see what the compiler says? (We hit > > this a lot in QUIC, and I *thought* that I understood the rules, but who > knows! > > :>) > > Ping. Unsigned is consider the larger type. Passing a large type unsigned into a smaller signed parameter induces a warning... but maybe we don't enable it (anymore??). I made the change... and ran the try bots... and I'll see what the various settings say. If it can stay green, I'd much rather avoid the cast. https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... net/quic/port_suggester_unittest.cc:59: // When the range is large, duplicates are rare, but we'll ask for a few On 2013/12/07 04:52:22, Ryan Hamilton wrote: > On 2013/12/07 03:12:18, jar wrote: > > On 2013/12/07 01:46:14, Ryan Hamilton wrote: > > > There have been a few threads on chromium-dev about making sure that > "random" > > > tests don't occasionally fail. It looks like these tests are fine, because > > > you're seeding with a fixed constant, the behavior is totally consistent. > Is > > > that right? > > > > Exactly. All values and algorithms are fixed... > > Great! There is a tiny chance that some big-endian issue may bite us, as we move the hash into a uint64 (or similar via unions). In that case, we'll get a busted test... and need to find some set of tests and entropy that will work on both architectures... but it shouldn't be hard to find such. https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/port_su... File net/quic/port_suggester.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/port_su... net/quic/port_suggester.cc:40: previous_suggestion_ = static_cast<int>(seed_ % range) + min; On 2013/12/07 23:21:55, Ryan Hamilton wrote: > I'm not sure that I understand the need for previous_suggestion_. Can you > simply do: > > return (seed_ % range) + min; > > This avoids the need for previous_suggestion_ at all? > > edit: Oh, I see, it's used in the DCHECK in quic_stream_factory.cc. > > nit: If you decide to keep the unit test, you might consider implementing > previous_suggestion() as: > return (seed_ % range) + min; > instead of dedicating a member variable to this function which is only used in a > DCHECK (and hence not in the production binary). You could return > previous_suggestion(); as the last line from SuggestPort to reuse the code. Min and max are not members... so the calculation can't be repeated separately. The method will be optimized out (as unused) in production code... which should indeed cause the slot to be optimized out as never referenced. That said, the object is dynamic, and pretty short lived, so the the extra 4 bytes won't be a big issue. https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/port_su... File net/quic/port_suggester.h (right): https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/port_su... net/quic/port_suggester.h:23: class NET_EXPORT PortSuggester On 2013/12/07 23:21:55, Ryan Hamilton wrote: > Should this be NET_EXPORT or NET_EXPORT_PRIVATE? If you do not intend to use it > outside of net/, I think you can make it PRIVATE. Done. https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/port_su... net/quic/port_suggester.h:24: : public base::RefCounted<PortSuggester> { On 2013/12/07 23:21:55, Ryan Hamilton wrote: > nit: I think this can all fit on a single line. (Unless you make it private :>) Made private... doesn't fit. https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/port_su... net/quic/port_suggester.h:34: int previous_suggestion() const { return previous_suggestion_; } On 2013/12/07 23:21:55, Ryan Hamilton wrote: > if you decide to keep this method, should it DCHECK_LT(0u, call_count_)? I can add that. I gave it a init value of -1, so that it would be sure to fail when compared with plausible ports that get use. I'll add the DCHECK(). https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/port_su... net/quic/port_suggester.h:42: uint64 seed_; On 2013/12/07 23:21:55, Ryan Hamilton wrote: > nit: extra space before seed_; Done. https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/port_su... File net/quic/port_suggester_unittest.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/port_su... net/quic/port_suggester_unittest.cc:19: : entropy_(1345689), On 2013/12/07 23:21:55, Ryan Hamilton wrote: > Out of curiosity, did the previous seed produce problematic results? Yes. As I changed the PRNG, there were different failures, and I needed to go with different entropy values. The PRNG is far from perfect, so it is not too hard to get a collision when stressed with a few hundred simultaneous calls. Keep in mind that the test restricted the output results to roughly 2^16 (port values), and so we expect that around 2^8 we'll have a 50% chance of a collision. https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/quic_st... File net/quic/quic_stream_factory.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/quic_st... net/quic/quic_stream_factory.cc:464: address.port() == port_suggester->previous_suggestion()); On 2013/12/07 23:21:55, Ryan Hamilton wrote: > I don't think that this DCHECK is guaranteed to succeed, is it? If we get > *really* unlikely the kBindRetries ports suggested by the suggester may all be > taken. As I understand it, at that point we do a default bind (with the > suggestion of 0). > > for (int i = 0; i < kBindRetries; ++i) { > int rv = DoBind(IPEndPoint(ip, rand_int_cb_.Run(kPortStart, kPortEnd))); > if (rv == OK || rv != ERR_ADDRESS_IN_USE) > return rv; > } > return DoBind(IPEndPoint(ip, 0)); > > Given this, I think you should remove this DCHECK (and possibly > previous_suggestion(), since it would then become unused) I thought the chance of exhausting the 10 attempts was pretty unlikely (in a debug build). I added an extra test that assures it is correct (it only validates the result when the port_suggester was not called many times. The reason why I especially liked this DCHECK was that I wanted to be sure that the PRNG was getting called on each platform. The previous cade had the bug that it was never calling the PRNG anywhere.. and I noticed that each platform (win vs posix vs other?) makes separate use of the PRNG... and I guessed it would be easy for some platform to not bother getting this right (even now that *we* called with the right argument).
Looking good. Down to one nit, and some questions about the DCHECK in the QuicStreamFactory. https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... File net/quic/port_suggester_unittest.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... net/quic/port_suggester_unittest.cc:51: if (ports.size() == static_cast<size_t>(port_range)) { On 2013/12/08 01:38:36, jar wrote: > On 2013/12/07 23:21:54, Ryan Hamilton wrote: > > On 2013/12/07 04:52:22, Ryan Hamilton wrote: > > > On 2013/12/07 03:12:18, jar wrote: > > > > On 2013/12/07 01:46:14, Ryan Hamilton wrote: > > > > > instead of casting, how about making port_range a size_t? > > > > > > > > ...then I'd have to cast when I pass the range into the SuggestPort() > > > function, > > > > that takes two ints :-/. > > > > > > Are you sure that's right? In my experience it's not a problem to pass in > > > unsigned values to signed functions. The problem comes when you *compare* > > > signed to unsigned. Can you switch and see what the compiler says? (We hit > > > this a lot in QUIC, and I *thought* that I understood the rules, but who > > knows! > > > :>) > > > > Ping. > > Unsigned is consider the larger type. Passing a large type unsigned into a > smaller signed parameter induces a warning... but maybe we don't enable it > (anymore??). *nod* As far as I know, we do not warn/error on narrowing conversions, only on signed/unsigned comparison. But every time I think I know what's going on, I learn something new, so the bots will tell :> > I made the change... and ran the try bots... and I'll see what the various > settings say. > > If it can stay green, I'd much rather avoid the cast. https://chromiumcodereview.appspot.com/107803002/diff/100001/net/quic/port_su... net/quic/port_suggester_unittest.cc:59: // When the range is large, duplicates are rare, but we'll ask for a few On 2013/12/08 01:38:36, jar wrote: > On 2013/12/07 04:52:22, Ryan Hamilton wrote: > > On 2013/12/07 03:12:18, jar wrote: > > > On 2013/12/07 01:46:14, Ryan Hamilton wrote: > > > > There have been a few threads on chromium-dev about making sure that > > "random" > > > > tests don't occasionally fail. It looks like these tests are fine, > because > > > > you're seeding with a fixed constant, the behavior is totally consistent. > > Is > > > > that right? > > > > > > Exactly. All values and algorithms are fixed... > > > > Great! > > There is a tiny chance that some big-endian issue may bite us, as we move the > hash into a uint64 (or similar via unions). In that case, we'll get a busted > test... and need to find some set of tests and entropy that will work on both > architectures... but it shouldn't be hard to find such. SGTM https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/port_su... File net/quic/port_suggester.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/port_su... net/quic/port_suggester.cc:40: previous_suggestion_ = static_cast<int>(seed_ % range) + min; On 2013/12/08 01:38:36, jar wrote: > On 2013/12/07 23:21:55, Ryan Hamilton wrote: > > I'm not sure that I understand the need for previous_suggestion_. Can you > > simply do: > > > > return (seed_ % range) + min; > > > > This avoids the need for previous_suggestion_ at all? > > > > edit: Oh, I see, it's used in the DCHECK in quic_stream_factory.cc. > > > > nit: If you decide to keep the unit test, you might consider implementing > > previous_suggestion() as: > > return (seed_ % range) + min; > > instead of dedicating a member variable to this function which is only used in > a > > DCHECK (and hence not in the production binary). You could return > > previous_suggestion(); as the last line from SuggestPort to reuse the code. > > Min and max are not members... so the calculation can't be repeated separately. Oh, right! Of course, that's obvious now that you point it out. > The method will be optimized out (as unused) in production code... which should > indeed cause the slot to be optimized out as never referenced. That said, the > object is dynamic, and pretty short lived, so the the extra 4 bytes won't be a > big issue. *nod* https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/quic_st... File net/quic/quic_stream_factory.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/quic_st... net/quic/quic_stream_factory.cc:464: address.port() == port_suggester->previous_suggestion()); On 2013/12/08 01:38:36, jar wrote: > On 2013/12/07 23:21:55, Ryan Hamilton wrote: > > I don't think that this DCHECK is guaranteed to succeed, is it? If we get > > *really* unlikely the kBindRetries ports suggested by the suggester may all be > > taken. As I understand it, at that point we do a default bind (with the > > suggestion of 0). > > > > for (int i = 0; i < kBindRetries; ++i) { > > int rv = DoBind(IPEndPoint(ip, rand_int_cb_.Run(kPortStart, kPortEnd))); > > if (rv == OK || rv != ERR_ADDRESS_IN_USE) > > return rv; > > } > > return DoBind(IPEndPoint(ip, 0)); > > > > Given this, I think you should remove this DCHECK (and possibly > > previous_suggestion(), since it would then become unused) > > I thought the chance of exhausting the 10 attempts was pretty unlikely (in a > debug build). > > I added an extra test that assures it is correct (it only validates the result > when the port_suggester was not called many times. > > The reason why I especially liked this DCHECK was that I wanted to be sure that > the PRNG was getting called on each platform. The previous cade had the bug > that it was never calling the PRNG anywhere.. and I noticed that each platform > (win vs posix vs other?) makes separate use of the PRNG... and I guessed it > would be easy for some platform to not bother getting this right (even now that > *we* called with the right argument). Hm. I'm totally down with a DCHECK to make sure this is working, since a bona-fide test is highly non-trivial. (Perhaps I'll work on that in my copious spare time :>). However, I'm very uncomfortable with landing DCHECKs that we know can fail (albeit under unlikely circumstances). In particular, I'm extremely reluctant to do have code that might occasionally break the tree. As a sheriff, those kinds of failures drive me nuts! You could consider augmenting the check to make sure that fewer than kBindRetries have been attempted? But at that point, we're starting to get a really convoluted set of conditions. By the way, when we use MockSockets, the rand_in_cb is not invoked at all. Does this DCHECK fail in those situations? https://chromiumcodereview.appspot.com/107803002/diff/190001/net/quic/port_su... File net/quic/port_suggester_unittest.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/190001/net/quic/port_su... net/quic/port_suggester_unittest.cc:56: EXPECT_EQ(static_cast<size_t>(port_range), ports.size()); nit: I think you can remove this cast, since port_range is unsigned, it will not cause problems.
https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/quic_st... File net/quic/quic_stream_factory.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/quic_st... net/quic/quic_stream_factory.cc:464: address.port() == port_suggester->previous_suggestion()); On 2013/12/08 04:00:29, Ryan Hamilton wrote: > On 2013/12/08 01:38:36, jar wrote: > > On 2013/12/07 23:21:55, Ryan Hamilton wrote: > > > I don't think that this DCHECK is guaranteed to succeed, is it? If we get > > > *really* unlikely the kBindRetries ports suggested by the suggester may all > be > > > taken. As I understand it, at that point we do a default bind (with the > > > suggestion of 0). > > > > > > for (int i = 0; i < kBindRetries; ++i) { > > > int rv = DoBind(IPEndPoint(ip, rand_int_cb_.Run(kPortStart, kPortEnd))); > > > if (rv == OK || rv != ERR_ADDRESS_IN_USE) > > > return rv; > > > } > > > return DoBind(IPEndPoint(ip, 0)); > > > > > > Given this, I think you should remove this DCHECK (and possibly > > > previous_suggestion(), since it would then become unused) > > > > I thought the chance of exhausting the 10 attempts was pretty unlikely (in a > > debug build). > > > > I added an extra test that assures it is correct (it only validates the result > > when the port_suggester was not called many times. > > > > The reason why I especially liked this DCHECK was that I wanted to be sure > that > > the PRNG was getting called on each platform. The previous cade had the bug > > that it was never calling the PRNG anywhere.. and I noticed that each platform > > (win vs posix vs other?) makes separate use of the PRNG... and I guessed it > > would be easy for some platform to not bother getting this right (even now > that > > *we* called with the right argument). > > Hm. I'm totally down with a DCHECK to make sure this is working, since a > bona-fide test is highly non-trivial. (Perhaps I'll work on that in my copious > spare time :>). However, I'm very uncomfortable with landing DCHECKs that we > know can fail (albeit under unlikely circumstances). In particular, I'm > extremely reluctant to do have code that might occasionally break the tree. As > a sheriff, those kinds of failures drive me nuts! You could consider augmenting > the check to make sure that fewer than kBindRetries have been attempted? But at > that point, we're starting to get a really convoluted set of conditions. > > By the way, when we use MockSockets, the rand_in_cb is not invoked at all. Does > this DCHECK fail in those situations? Please take a look at the DCHECK that was in the most recent version. I think it is very conservative, and I don't see a way in which it can break bots. If bots make it "hard to connect" then the suggest will be called 10 times, and the DCHECK will have long since passed. Currently, this DCHECK does not break any tests that I'm aware of (as can be seen by the try bots). Can you tell me how I should run such a test? Note that IF we have mocked out tests, and they start to trigger this DCHECK, all we'd have to do is call the cb more than 2 times... and the DCHECK would be "satisfied." https://chromiumcodereview.appspot.com/107803002/diff/190001/net/quic/port_su... File net/quic/port_suggester_unittest.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/190001/net/quic/port_su... net/quic/port_suggester_unittest.cc:56: EXPECT_EQ(static_cast<size_t>(port_range), ports.size()); On 2013/12/08 04:00:29, Ryan Hamilton wrote: > nit: I think you can remove this cast, since port_range is unsigned, it will not > cause problems. Done.
https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/quic_st... File net/quic/quic_stream_factory.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/quic_st... net/quic/quic_stream_factory.cc:464: address.port() == port_suggester->previous_suggestion()); On 2013/12/08 04:57:43, jar wrote: > On 2013/12/08 04:00:29, Ryan Hamilton wrote: > > On 2013/12/08 01:38:36, jar wrote: > > > On 2013/12/07 23:21:55, Ryan Hamilton wrote: > > > > I don't think that this DCHECK is guaranteed to succeed, is it? If we get > > > > *really* unlikely the kBindRetries ports suggested by the suggester may > all > > be > > > > taken. As I understand it, at that point we do a default bind (with the > > > > suggestion of 0). > > > > > > > > for (int i = 0; i < kBindRetries; ++i) { > > > > int rv = DoBind(IPEndPoint(ip, rand_int_cb_.Run(kPortStart, > kPortEnd))); > > > > if (rv == OK || rv != ERR_ADDRESS_IN_USE) > > > > return rv; > > > > } > > > > return DoBind(IPEndPoint(ip, 0)); > > > > > > > > Given this, I think you should remove this DCHECK (and possibly > > > > previous_suggestion(), since it would then become unused) > > > > > > I thought the chance of exhausting the 10 attempts was pretty unlikely (in a > > > debug build). > > > > > > I added an extra test that assures it is correct (it only validates the > result > > > when the port_suggester was not called many times. > > > > > > The reason why I especially liked this DCHECK was that I wanted to be sure > > that > > > the PRNG was getting called on each platform. The previous cade had the bug > > > that it was never calling the PRNG anywhere.. and I noticed that each > platform > > > (win vs posix vs other?) makes separate use of the PRNG... and I guessed it > > > would be easy for some platform to not bother getting this right (even now > > that > > > *we* called with the right argument). > > > > Hm. I'm totally down with a DCHECK to make sure this is working, since a > > bona-fide test is highly non-trivial. (Perhaps I'll work on that in my > copious > > spare time :>). However, I'm very uncomfortable with landing DCHECKs that we > > know can fail (albeit under unlikely circumstances). In particular, I'm > > extremely reluctant to do have code that might occasionally break the tree. > As > > a sheriff, those kinds of failures drive me nuts! You could consider > augmenting > > the check to make sure that fewer than kBindRetries have been attempted? But > at > > that point, we're starting to get a really convoluted set of conditions. > > > > By the way, when we use MockSockets, the rand_in_cb is not invoked at all. > Does > > this DCHECK fail in those situations? > > Please take a look at the DCHECK that was in the most recent version. I think > it is very conservative, and I don't see a way in which it can break bots. If > bots make it "hard to connect" then the suggest will be called 10 times, and the > DCHECK will have long since passed. > > Currently, this DCHECK does not break any tests that I'm aware of (as can be > seen by the try bots). > > Can you tell me how I should run such a test? > > Note that IF we have mocked out tests, and they start to trigger this DCHECK, > all we'd have to do is call the cb more than 2 times... and the DCHECK would be > "satisfied." I totally believe the DCHECK is passing on the bots, I'm just not sure what it does :> Can you help me understand? I feel like I'm screwing up the polarity of the conditions. 467 DCHECK(ERR_SOCKET_NOT_CONNECTED != socket->GetLocalAddress(&address) || I'm not sure I'm reading this first clause correctly, but this seems to say that if the socket is actually connected, then we don't do the rest of the checks. Am I misreading it? If not, is this the right behavior? The test I'm thinking of is QuicStreamFactoryTest. I patched in your CL and notice that when I run that test, the suggester's call_count is 0. However, the DCHECK does not trigger. This seems to be because the first condition is true... the socket is actually connected. Is this doing what you expect?
https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/quic_st... File net/quic/quic_stream_factory.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/160002/net/quic/quic_st... net/quic/quic_stream_factory.cc:464: address.port() == port_suggester->previous_suggestion()); On 2013/12/08 05:24:08, Ryan Hamilton wrote: > On 2013/12/08 04:57:43, jar wrote: > > On 2013/12/08 04:00:29, Ryan Hamilton wrote: > > > On 2013/12/08 01:38:36, jar wrote: > > > > On 2013/12/07 23:21:55, Ryan Hamilton wrote: > > > > > I don't think that this DCHECK is guaranteed to succeed, is it? If we > get > > > > > *really* unlikely the kBindRetries ports suggested by the suggester may > > all > > > be > > > > > taken. As I understand it, at that point we do a default bind (with the > > > > > suggestion of 0). > > > > > > > > > > for (int i = 0; i < kBindRetries; ++i) { > > > > > int rv = DoBind(IPEndPoint(ip, rand_int_cb_.Run(kPortStart, > > kPortEnd))); > > > > > if (rv == OK || rv != ERR_ADDRESS_IN_USE) > > > > > return rv; > > > > > } > > > > > return DoBind(IPEndPoint(ip, 0)); > > > > > > > > > > Given this, I think you should remove this DCHECK (and possibly > > > > > previous_suggestion(), since it would then become unused) > > > > > > > > I thought the chance of exhausting the 10 attempts was pretty unlikely (in > a > > > > debug build). > > > > > > > > I added an extra test that assures it is correct (it only validates the > > result > > > > when the port_suggester was not called many times. > > > > > > > > The reason why I especially liked this DCHECK was that I wanted to be sure > > > that > > > > the PRNG was getting called on each platform. The previous cade had the > bug > > > > that it was never calling the PRNG anywhere.. and I noticed that each > > platform > > > > (win vs posix vs other?) makes separate use of the PRNG... and I guessed > it > > > > would be easy for some platform to not bother getting this right (even now > > > that > > > > *we* called with the right argument). > > > > > > Hm. I'm totally down with a DCHECK to make sure this is working, since a > > > bona-fide test is highly non-trivial. (Perhaps I'll work on that in my > > copious > > > spare time :>). However, I'm very uncomfortable with landing DCHECKs that > we > > > know can fail (albeit under unlikely circumstances). In particular, I'm > > > extremely reluctant to do have code that might occasionally break the tree. > > As > > > a sheriff, those kinds of failures drive me nuts! You could consider > > augmenting > > > the check to make sure that fewer than kBindRetries have been attempted? > But > > at > > > that point, we're starting to get a really convoluted set of conditions. > > > > > > By the way, when we use MockSockets, the rand_in_cb is not invoked at all. > > Does > > > this DCHECK fail in those situations? > > > > Please take a look at the DCHECK that was in the most recent version. I think > > it is very conservative, and I don't see a way in which it can break bots. If > > bots make it "hard to connect" then the suggest will be called 10 times, and > the > > DCHECK will have long since passed. > > > > Currently, this DCHECK does not break any tests that I'm aware of (as can be > > seen by the try bots). > > > > Can you tell me how I should run such a test? > > > > Note that IF we have mocked out tests, and they start to trigger this DCHECK, > > all we'd have to do is call the cb more than 2 times... and the DCHECK would > be > > "satisfied." > > I totally believe the DCHECK is passing on the bots, I'm just not sure what it > does :> Can you help me understand? I feel like I'm screwing up the polarity > of the conditions. > > 467 DCHECK(ERR_SOCKET_NOT_CONNECTED != socket->GetLocalAddress(&address) || > > I'm not sure I'm reading this first clause correctly, but this seems to say that > if the socket is actually connected, then we don't do the rest of the checks. > Am I misreading it? If not, is this the right behavior? > > The test I'm thinking of is QuicStreamFactoryTest. I patched in your CL and > notice that when I run that test, the suggester's call_count is 0. However, the > DCHECK does not trigger. This seems to be because the first condition is > true... the socket is actually connected. Is this doing what you expect? Something that might make the DCHECK a bit more clear would be to structure it like: if (socket->GetLocalAddress(&address) == OK && // (or whatever this clause is expected to be) port_suggester->call_count() <= 2) { DCHECK_EQ(port_suggester->previous_suggestion(), address.port()); } It's a bit easier to read the intend, and in the event that it fires, you'll get to see both values. For example, if suggestions is -1, that means that it wasn't called at all..
I made two changes in quic_test_util.cc to allow the DCHECK in quic_factory_stream_test.cc to work. Ryan: Is this acceptable, or do you have a better suggestion? Being wary that some platforms might suggest (at least sometimes) unacceptable ports (due to range restrictions), it seemed nice to still validate for both 1 and 2 suggestions. This also stayed way under the current limits of 10 tries (before going to OS suggestions) in both the posix and windows implementations. https://chromiumcodereview.appspot.com/107803002/diff/220001/net/quic/quic_st... File net/quic/quic_stream_factory.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/220001/net/quic/quic_st... net/quic/quic_stream_factory.cc:475: port_suggester->call_count() > 2 || The polarity of the first clause (line 474) was wrong, which mostly neutered the DCHECK. When I corrected the polarity, then some tests in quic_factory_stream_test.cc do indeed fail... <yeah!> ...and the proposed technique of calling the PRNG several times in the Mock socket, allows this DCHECK to be fine.
It looks like the tests I fixed on Window to deal with the assertion (I'm guessing the assertion is the issue... as it is now de-neutered) are no where near sufficient. A pile of additional tests are failing on Linux try bots. I'm looking into it. On Sun, Dec 8, 2013 at 1:12 PM, <jar@chromium.org> wrote: > I made two changes in quic_test_util.cc to allow the DCHECK in > quic_factory_stream_test.cc to work. > > Ryan: Is this acceptable, or do you have a better suggestion? > > Being wary that some platforms might suggest (at least sometimes) > unacceptable > ports (due to range restrictions), it seemed nice to still validate for > both 1 > and 2 suggestions. This also stayed way under the current limits of 10 > tries > (before going to OS suggestions) in both the posix and windows > implementations. > > > https://chromiumcodereview.appspot.com/107803002/diff/ > 220001/net/quic/quic_stream_factory.cc > File net/quic/quic_stream_factory.cc (right): > > https://chromiumcodereview.appspot.com/107803002/diff/ > 220001/net/quic/quic_stream_factory.cc#newcode475 > net/quic/quic_stream_factory.cc:475: port_suggester->call_count() > 2 || > The polarity of the first clause (line 474) was wrong, which mostly > neutered the DCHECK. > > When I corrected the polarity, then some tests in > quic_factory_stream_test.cc do indeed fail... <yeah!> > > ...and the proposed technique of calling the PRNG several times in the > Mock socket, allows this DCHECK to be fine. > > https://chromiumcodereview.appspot.com/107803002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I'm completely happy with port_suggester*{h,cc}. Woo hoo! Thanks for the
fixes.
I recommend a different approach for the changes to socket_test_util.
https://chromiumcodereview.appspot.com/107803002/diff/220001/net/quic/quic_st...
File net/quic/quic_stream_factory.cc (right):
https://chromiumcodereview.appspot.com/107803002/diff/220001/net/quic/quic_st...
net/quic/quic_stream_factory.cc:475: port_suggester->call_count() > 2 ||
On 2013/12/08 21:12:58, jar wrote:
> The polarity of the first clause (line 474) was wrong, which mostly neutered
the
> DCHECK.
>
> When I corrected the polarity, then some tests in quic_factory_stream_test.cc
do
> indeed fail... <yeah!>
Yay for tests :>
> ...and the proposed technique of calling the PRNG several times in the Mock
> socket, allows this DCHECK to be fine.
https://chromiumcodereview.appspot.com/107803002/diff/240001/net/quic/quic_st...
File net/quic/quic_stream_factory.cc (right):
https://chromiumcodereview.appspot.com/107803002/diff/240001/net/quic/quic_st...
net/quic/quic_stream_factory.cc:470: // validate the source port.
Non-test code should not have to be written with knowledge of the implementation
details of test-only code. I have a suggestion in socket_test_utils.cc
https://chromiumcodereview.appspot.com/107803002/diff/240001/net/quic/quic_st...
net/quic/quic_stream_factory.cc:474: DCHECK(ERR_SOCKET_NOT_CONNECTED ==
socket->GetLocalAddress(&address) ||
I think that instead of checking the specific error code, it make more sense to
check != OK.
nit: It is not idiomatic in net/ to put the constant before the == and we pretty
much always do it the other way. (All of the instances of == in this file have
the constant after the ==). Please reverse the order to be consistent.
https://chromiumcodereview.appspot.com/107803002/diff/240001/net/socket/socke...
File net/socket/socket_test_util.cc (right):
https://chromiumcodereview.appspot.com/107803002/diff/240001/net/socket/socke...
net/socket/socket_test_util.cc:675:
ExerciseSocketRandomNumberGenerator(rand_int_cb);
Instead of invoking the callback simply to work around, the DCHECK I recommend a
different approach.
* add a port_ member to this class, and initialize it to, say, 123 (which is the
existing port returned by GetLocalAddress).
* Modify GetLocalAddress to use port_ in the returned value.
* If |bind_type| is RANDOM_BIND, then set port_ to the result of invoking
|rand_int_cb|.
This has the virtue of correctly honoring |bind_type| and makes this method (and
GetLocalAdress) act like the "real" version.
On 2013/12/08 22:18:04, jar wrote: > It looks like the tests I fixed on Window to deal with the assertion (I'm > guessing the assertion is the issue... as it is now de-neutered) are no > where near sufficient. A pile of additional tests are failing on Linux try > bots. > > I'm looking into it. Doh! Good luck. I'll keep my eyes peeled.
LGTM after two small fixes. https://chromiumcodereview.appspot.com/107803002/diff/260001/net/quic/quic_st... File net/quic/quic_stream_factory.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/260001/net/quic/quic_st... net/quic/quic_stream_factory.cc:479: #endif // 0 Please go ahead and remove this code for now. You can keep it around in your work area while we figure out what's going on. https://chromiumcodereview.appspot.com/107803002/diff/260001/net/socket/socke... File net/socket/socket_test_util.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/260001/net/socket/socke... net/socket/socket_test_util.cc:133: } Please revert the changes in this file until we're ready to land the DCHECK again.
https://chromiumcodereview.appspot.com/107803002/diff/260001/net/quic/quic_st... File net/quic/quic_stream_factory.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/260001/net/quic/quic_st... net/quic/quic_stream_factory.cc:479: #endif // 0 On 2013/12/08 23:22:21, Ryan Hamilton wrote: > Please go ahead and remove this code for now. You can keep it around in your > work area while we figure out what's going on. Done. https://chromiumcodereview.appspot.com/107803002/diff/260001/net/socket/socke... File net/socket/socket_test_util.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/260001/net/socket/socke... net/socket/socket_test_util.cc:133: } On 2013/12/08 23:22:21, Ryan Hamilton wrote: > Please revert the changes in this file until we're ready to land the DCHECK > again. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/107803002/280001
Message was sent while issue was closed.
Change committed as 239426
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/107803002/diff/240001/net/socket/socke... File net/socket/socket_test_util.cc (right): https://chromiumcodereview.appspot.com/107803002/diff/240001/net/socket/socke... net/socket/socket_test_util.cc:675: ExerciseSocketRandomNumberGenerator(rand_int_cb); On 2013/12/08 22:54:15, Ryan Hamilton wrote: > Instead of invoking the callback simply to work around, the DCHECK I recommend a > different approach. > > * add a port_ member to this class, and initialize it to, say, 123 (which is the > existing port returned by GetLocalAddress). > * Modify GetLocalAddress to use port_ in the returned value. > * If |bind_type| is RANDOM_BIND, then set port_ to the result of invoking > |rand_int_cb|. > > This has the virtue of correctly honoring |bind_type| and makes this method (and > GetLocalAdress) act like the "real" version. I realized that if you make this change, then it should be pretty easy to write a unit test of QuicStreamFactory that makes two consecutive connections to the same server and verifies that the same port was used. You could then make a a connection to a different server, and verify that the port is different..
Message was sent while issue was closed.
This is a reland of the port_suggester code, atop the fixes landed yesterday.
LGTM Please add a unit test to quic_stream_factory which makes 2 sessions to host A and verifies that the port is the same, then makes a session to host B and verify that it has a different port. Of course, you can't do this until you land your changes to socket_test_util, but when that changes goes it, please add this test.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jar@chromium.org/107803002/310001
Patch set 19 LGTM. https://chromiumcodereview.appspot.com/107803002/diff/310001/net/quic/quic_st... File net/quic/quic_stream_factory.h (right): https://chromiumcodereview.appspot.com/107803002/diff/310001/net/quic/quic_st... net/quic/quic_stream_factory.h:239: uint64 port_entropy_; Nit: maybe this member should be renamed "port_seed_" now.
Message was sent while issue was closed.
Change committed as 240782 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
