Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(473)

Side by Side Diff: net/docs/life-of-a-feature.md

Issue 2700673002: [net] Add documentation for feature process & supported platforms (Closed)
Patch Set: Feedback Created 3 years, 10 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View unified diff | Download patch
« no previous file with comments | « no previous file | net/docs/supported-projects.md » ('j') | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
OLDNEW
(Empty)
1 # Life of a Feature
2
3 In the years since the Chromium browser was first open-sourced, the `//net`
4 directory has expanded from being the basis of loading web content in the
5 Chromium browser to accommodating a wide variety of networking needs,
6 both in the Chromium browser and in other Google and third-party products
7 and projects.
8
9 This brings with it many new opportunities, such as the ability to
10 introduce new protocols rapidly or push Web security forward, as well as
11 new challenges, such as how to balance the needs of various `//net`
12 consumers effectively.
13
14 To make it easier to contribute new features or to change existing
15 behaviors in `//net`, this document tries to capture the life of a
16 feature in `//net`, from initial design to the eventual possibility of
17 deprecation and removal.
18
19 ## Supported Projects
20
21 When considering the introduction of a new `//net` feature or changing
22 a `//net` behavior, it's first necessary to understand where `//net`
23 is used, how it is used, and what the various constraints and limits are.
24
25 To understand a more comprehensive matrix of the supported platforms and
26 constraints, see [Supported Projects](supported-projects.md). When
27 examining a new feature request, or a change in behavior, it's necessary
28 to consider dimensions such as:
29
30 * Does this feature apply to all supported projects, or is this something
31 like a Browser-only feature?
32 * Does this feature apply to all supported platforms, or is this something
33 specific to a particular subset?
34 * Is the feature a basic networking library feature, or is it specific to
35 something in the Web Platform?
36 * Will some projects wish to strip the feature in order to meet targets
37 such as memory usage (RAM) or binary size?
38 * Does it depend on Google services or Google-specific behaviors or
39 features?
40 * How will this feature be tested / experimented with? For example,
41 __Field Trials (Finch)__ and __User Metrics (UMA)__ may not be available
42 on a number of platforms.
43 * How risky is the feature towards compatibility/stability? How will it
44 be undone if there is a bug?
45 * Are the power/memory/CPU/bandwidth requirements appropriate for the
46 targeted projects and/or platforms?
47
48 ## Design and Layering
49
50 Once the supported platforms and constraints are identified, it's necessary
51 to determine how to actually design the feature to meet those constraints,
52 in hopefully the easiest way possible both for implementation and consumption.
53
54 ### Designing for multiple platforms
55
56 In general, `//net` features try to support all platforms with a common
57 interface, and generally eschew OS-specific interfaces from being exposed as
58 part of `//net`.
59
60 Cross-platform code is generally done via declaring an interface named
61 `foo.h`, which is common for all platforms, and then using the build-system to
62 do compile-time switching between implementations in `foo_win.cc`, `foo_mac.cc`,
63 `foo_android.cc`, etc.
64
65 The goal is to ensure that consumers generally don't have to think about
66 OS-specific considerations, and can instead code to the interface.
67
68 ### Designing for multiple products
69
70 While premature abstraction can significantly harm readability, if it is
71 anticipated that different products will have different implementation needs,
72 or may wish to selectively disable the feature, it's often necessary to
73 abstract that interface sufficiently in `//net` to allow for dependency
74 injection.
75
76 This is true whether discussing concrete classes and interfaces or something
77 as simple a boolean configuration flag that different consumers wish to set
78 differently.
79
80 The two most common approaches in `//net` are injection and delegation.
81
82 #### Injection
83
84 Injection refers to the pattern of defining the interface or concrete
85 configuration parameter (such as a boolean), along with the concrete
86 implementation, but requiring the `//net` embedder to supply an instance
87 of the interface or the configuration parameters (perhaps optionally).
88
89 Examples of this pattern include things such as the `ProxyConfigService`,
90 which has concrete implementations in `//net` for a variety of platforms'
91 configuration of proxies, but which requires it be supplied as part of the
92 `URLRequestContextGetter` building, if proxies are going to be supported.
93
94 An example of injecting configuration flags can be seen in the
95 `HttpNetworkSession::Params` structure, which is used to provide much of
96 the initialization parameters for the HTTP layer.
97
98 The ideal form of injection is to pass ownership of the injected object,
99 such as via a `std::unique_ptr<Foo>`. While this is not consistently
100 applied in `//net`, as there are a number of places in which ownership is
101 either shared or left to the embedder, with the injected object passed
102 around as a naked/raw pointer, this is generally seen as an anti-pattern
103 and not to be mirrored for new features.
104
105 #### Delegation
106
107 Delegation refers to forcing the `//net` embedder to respond to specific
108 delegated calls via a Delegate interface that it implements. In general,
109 when using the delegate pattern, ownership of the delegate should be
110 transferred, so that the lifetime and threading semantics are clear and
111 unambiguous.
112
113 That is, for a given class `Foo`, which has a `Foo::Delegate` interface
114 defined to allow embedders to alter behavior, prefer a constructor that
115 is
116 ```
117 explicit Foo(std::unique_ptr<Delegate> delegate);
118 ```
119 so that it is clear that the lifetime of `delegate` is determined by
120 `Foo`.
121
122 While this may appear similar to Injection, the general difference
123 between the two approaches is determining where the bulk of the
124 implementation lies. With Injection, the interface describes a
125 behavior contract that concrete implementations must adhere to; this
126 allows for much more flexibility with behavior, but with the downside
127 of significantly more work to implement or extend. Delegation attempts
128 to keep the bulk of the implementation in `//net`, and the decision as
129 to which implementation to use in `//net`, but allows `//net` to
130 provide specific ways in which embedders can alter behaviors.
131
132 The most notable example of the delegate pattern is `URLRequest::Delegate`,
133 which keeps the vast majority of the loading logic within `URLRequest`,
134 but allows the `URLRequest::Delegate` to participate during specific times
135 in the request lifetime and alter specific behaviors as necessary. (Note:
136 `URLRequest::Delegate`, like much of the original `//net` code, doesn't
137 adhere to the recommended lifetime patterns of passing ownership of the
138 Delegate. It is from the experience debugging and supporting these APIs
139 that the `//net` team strongly encourages all new code pass explicit
140 ownership, to reduce the complexity and risk of lifetime issues).
141
142 While the use of a `base::Callback` can also be considered a form of
143 delegation, the `//net` layer tries to eschew any callbacks that can be
144 called more than once, and instead favors defining class interfaces
145 with concrete behavioral requirements in order to ensure the correct
146 lifetimes of objects and to adjust over time. When `//net` takes a
147 callback (e.g. `net::CompletionCallback`), the intended pattern is to
148 signal the asynchronous completion of a single method, invoking that
149 callback at most once before deallocating it. For more discussion
150 of these patterns, see [Code Patterns](code-patterns.md).
151
152 ### Understanding the Layering
153
154 A significant challenge many feature proposals face is understanding the
155 layering in `//net` and what different portions of `//net` are allowed to
156 know.
157
158 #### Socket Pools
159
160 The most common challenge feature proposals encounter is the awareness
161 that the act of associating an actual request to make with a socket is
162 done lazily, referred to as "late-binding".
163
164 With late-bound sockets, a given `URLRequest` will not be assigned an actual
165 transport connection until the request is ready to be sent. This allows for
166 reprioritizing requests as they come in, to ensure that higher priority requests
167 get preferential treatment, but it also means that features or data associated
168 with a `URLRequest` generally don't participate in socket establishment or
169 maintenance.
170
171 For example, a feature that wants to associate the low-level network socket
172 with a `URLRequest` during connection establishment is not something that the
173 `//net` design supports, since the `URLRequest` is kept unaware of how sockets
174 are established by virtue of the socket pools and late binding. This allows for
175 more flexibility when working to improve performance, such as the ability to
176 coalesce multiple logical 'sockets' over a single HTTP/2 or QUIC stream, which
177 may only have a single physical network socket involved.
178
179 #### Making Additional Requests
180
181 From time to time, `//net` feature proposals will involve needing to load
182 a secondary resource as part of processing. For example, SDCH involves loading
183 additional dictionaries that are advertised in a header, and other feature
184 proposals have involved fetching a `/.well-known/` URI or reporting errors to
185 a particular URL.
186
187 This is particularly challenging, because often, these features are implemented
188 deeper in the network stack, such as [`//net/cert`](../cert), [`//net/http`](../ http),
189 or [`//net/filter`](../filter), which [`//net/url_request`](../url_request) depe nds
190 on. Because `//net/url_request` depends on these low-level directories, it would
191 be a circular dependency to have these directories depend on `//net/url_request` ,
192 and circular dependencies are forbidden.
193
194 The recommended solution to address this is to adopt the delegation or injection
195 patterns. The lower-level directory will define some interface that represents t he
196 "I need this URL" request, and then elsewhere, in a directory allowed to depend
197 on `//net/url_request`, an implementation of that interface/delegate that uses
198 `//net/url_request` is implemented.
199
200 ### Understanding the Lifetimes
201
202 Understanding the object lifetime and dependency graph can be one of the largest
203 challenges to contributing and maintaining `//net`. As a consequence, features
204 which require introducing more complexity to the lifetimes of objects generally
205 have a greater challenge to acceptance.
206
207 The `//net` stack is designed heavily around a sync-or-async pattern, as
208 documented in [Code Patterns](code-patterns.md), while also having a strong
209 requirement that it be possible to cleanly shutdown the network stack. As a
210 consequence, features should have precise, well-defined lifetime semantics
211 and support graceful cleanup. Further, because much of the network stack can
212 have web-observable side-effects, it is often required for tasks to have
213 defined sequencing that cannot be ordered. To be ensure these requirements
Randy Smith (Not in Mondays) 2017/02/21 17:01:04 nit: I don't understand what "defined sequencing t
214 are met, features should attempt to model object lifetimes as a hierarchical
215 DAG, using explicit ownership and avoiding the use of reference counting or
216 weak pointers as part of any of the exposed API contracts (even for features
217 only consumed in `//net`). Features that pay close attention to the lifetime
218 semantics are more likely to be reviewed and accepted than those that leave
219 it ambiguous.
220
221 In addition to preferring explicit lifetimes, such as through judicious use of
222 `std::unique_ptr<>` to indicate ownership transfer of dependencies, many
223 features in `//net` also expect that if a `base::Callback` is involved (which
224 includes `net::CompletionCallback`), then it's possible that invoking the
225 callback may result in the deletion of the current (calling) object. As
226 further expanded upon in [Code Patterns](code-patterns.md), features and
227 changes should be designed such that any callback invocation is the last
228 bit of code executed, and that the callback is accessed via the stack (such
229 as through the use of either `base::ResetAndReturn(callback_).Run()` or
230 `std::move(callback_).Run()`.
231
232 ### Specs: What Are They Good For
233
234 As `//net` is used as the basis for a number of browsers, it's an important part
235 of the design philosophy to ensure behaviors are well-specified, and that the
236 implementation conforms to those specifications. This may be seen as burdensome
237 when it's unclear whether or not a feature will 'take off,' but it's equally
238 critical to ensure that the Chromium projects do not fork the Web Platform.
239
240 #### Incubation Is Required
241
242 `//net` respects Chromium's overall position of [incubation first](https://group s.google.com/a/chromium.org/d/msg/blink-dev/PJ_E04kcFb8/baiLN3DTBgAJ) standards development.
243
244 With an incubation first approach, before introducing any new features that
245 might be exposed over the wire to servers, whether they are explicit behaviors,
246 such as adding new headers, or implicit behaviors such as
247 [Happy Eyeballs](https://tools.ietf.org/html/rfc6555), should have some form
248 of specification written. That specification should at least be on an
249 incubation track, and the expectation is that the specification should have a
250 direct path to an appropriate standards track. Features which don't adhere to
251 this pattern, or which are not making progress towards a standards track, will
252 require high-level approvals, to ensure that the Platform doesn't fragment.
253
254 #### Introducing New Headers
255
256 A common form of feature request is the introduction of new headers, either via
257 the `//net` implementation directly, or through consuming `//net` interfaces
258 and modifying headers on the fly.
259
260 The introduction of any additional headers SHOULD have an incubated spec attache d,
261 ideally with cross-vendor interest. Particularly, headers which only apply to
262 Google or Google services are very likely to be rejected outright.
263
264 #### Making Additional Requests
265
266 While it's necessary to provide abstraction around `//net/url_request` for
267 any lower-level components that may need to make additional requests, for most
268 features, that's not all that is necessary. Because `//net/url_request` only
269 provides a basic HTTP fetching mechanism, it's insufficient for any Web Platform
270 feature, because it doesn't consider the broader platform concerns such as
271 interactions with CORS, Service Workers, cookie and authentication policies, or
272 even basic interactions with optional features like Extensions or SafeBrowsing.
273
274 To account for all of these things, any resource fetching that is to support
275 a feature of the Web Platform, whether because the resource will be directly
276 exposed to web content (for example, an image load or prefetch) or because it
277 is in response to invoking a Web Platform API (for example, invoking the
278 credential management API), the feature's resource fetching should be
279 explainable in terms of the [Fetch Living Standard](https://fetch.spec.whatwg.o rg/).
280 The Fetch standard defines a JavaScript API for fetching resources, but more
281 importantly, defines a common set of infrastructure and terminology that
282 tries to define how all resource loads in the Web Platform happen - whether
283 it be through the JavaScript API, through `XMLHttpRequest`, or the `src`
284 attribute in HTML tags, for example.
285
286 This also includes any resource fetching that wishes to use the same socket
287 pools or caches as the Web Platform, to ensure that every resource that is
288 web exposed (directly or indirectly) is fetched in a consistent and
289 well-documented way, thus minimizing platform fragmentation and security
290 issues.
291
292 There are exceptions to this, however, but they're generally few and far
293 between. In general, any feature that needs to define an abstraction to
294 allow it to "fetch resources," likely needs to also be "explained in terms
295 of Fetch".
296
297 ## Implementation
298
299 In general, prior to implementing, try to get a review on net-dev@chromium.org
300 for the general feedback and design review.
301
302 In addition to the net-dev@chromium.org early review, `//net` requires that any
303 browser-exposed behavior should also adhere to the
304 [Blink Process](https://www.chromium.org/blink#new-features), which includes an
305 "Intent to Implement" message to blink-dev@chromium.org
306
307 For features that are unclear about their future, such as experiments or trials,
308 it's also expected that the design planning will also account for how features
309 will be removed cleanly. For features that radically affect the architecture of
310 `//net`, expect a high bar of justification, since reversing those changes if
311 they fail to pan out can cause significant disruption to productivity and
312 stability.
313
314 ## Deprecation
315
316 Plan for obsolence, hope for success. Similar to implementation, features that
317 are to be removed should also go through the
318 [Blink Process](https://www.chromium.org/blink#TOC-Web-Platform-Changes:-Process )
319 for removing features.
320
321 Note that due to the diversity of [Supported Projects](supported-projects.md),
322 removing a feature while minimizing disruption can be just as complex as adding
323 a feature. For example, relying solely on __User Metrics (UMA)__ to signal the
324 safety of removing a feature may not consider all projects, and relying on
325 __Field Trials (Finch)__ to assess risk or restore the 'legacy' behavior may not
326 work on all projects either.
327
328 It's precisely because of these challenges that there's such a high bar for
329 adding features, because they may represent multi-year commitments to support,
330 even when the feature itself is deprecated or targeted for removal.
OLDNEW
« no previous file with comments | « no previous file | net/docs/supported-projects.md » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698