Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
185313002:
restructure client api to use streams (Closed)
|
Created:
6 years, 9 months ago by danrubel Modified:
6 years, 9 months ago CC:
reviews_dartlang.org Visibility:
Public. |
Descriptionrestructure client api to use streams
+ restructure client api to use streams for notifications and responses
+ client waits for server shutdown response
+ cleanup channel tests
R=brianwilkerson@google.com
Committed: https://code.google.com/p/dart/source/detail?r=33330
Patch Set 1 #
Total comments: 4
Patch Set 2 : fix mock stream to more closely match websocket #Patch Set 3 : fix mock stream to more closely match websocket #
Total comments: 52
Patch Set 4 : address comments #
Messages
Total messages: 9 (0 generated)
LGTM, but I don't understand very much about streams yet. Might be good to find someone else that would be willing to look at our work until we're a little more proficient. https://codereview.chromium.org/185313002/diff/1/pkg/analysis_server/lib/src/... File pkg/analysis_server/lib/src/channel.dart (right): https://codereview.chromium.org/185313002/diff/1/pkg/analysis_server/lib/src/... pkg/analysis_server/lib/src/channel.dart:89: Stream jsonStream = _socket Does 'jsonStream' need to be a broadcast stream in order to be listened to by both the response and notification streams? https://codereview.chromium.org/185313002/diff/1/pkg/analysis_server/lib/src/... File pkg/analysis_server/lib/src/protocol.dart (right): https://codereview.chromium.org/185313002/diff/1/pkg/analysis_server/lib/src/... pkg/analysis_server/lib/src/protocol.dart:270: return null; Would it be better to throw an exception? (Not sure, just asking.)
https://codereview.chromium.org/185313002/diff/1/pkg/analysis_server/lib/src/... File pkg/analysis_server/lib/src/channel.dart (right): https://codereview.chromium.org/185313002/diff/1/pkg/analysis_server/lib/src/... pkg/analysis_server/lib/src/channel.dart:89: Stream jsonStream = _socket On 2014/03/01 17:16:37, Brian Wilkerson wrote: > Does 'jsonStream' need to be a broadcast stream in order to be listened to by > both the response and notification streams? Good catch! The mock stream was incorrectly simulating a broadcast stream whereas the real websocket stream is not. Fixed the mocks and added a test. https://codereview.chromium.org/185313002/diff/1/pkg/analysis_server/lib/src/... File pkg/analysis_server/lib/src/protocol.dart (right): https://codereview.chromium.org/185313002/diff/1/pkg/analysis_server/lib/src/... pkg/analysis_server/lib/src/protocol.dart:270: return null; On 2014/03/01 17:16:37, Brian Wilkerson wrote: > Would it be better to throw an exception? (Not sure, just asking.) I'm not sure either. Not in this CL, but this is something we should revisit.
I think the semantics here are OK, but I'm not an expert on streams. Added Nathan because he knows them better than I do. Also, as usual, I was unable to resist commenting on style. But overall, this looks pretty idiomatic to me. If you find yourself writing more complex tests than these, I encourage you to look at the scheduled_test package. It makes writing asynchronous test expectations *much* nicer. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... File pkg/analysis_server/lib/src/analysis_manager.dart (right): https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... pkg/analysis_server/lib/src/analysis_manager.dart:121: channel.close(); This should probably be async too: return channel.close().then((_) => false); https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... pkg/analysis_server/lib/src/analysis_manager.dart:142: }); The logic looks good here, but I would format it like: var request = new Request('0', ServerDomainHandler.SHUTDOWN_METHOD); var timeout = new Duration(seconds: 2); return channel.sendRequest(request).timeout(timeout, onTimeout: () { print('Expected shutdown response'); }).then((response) { channel.close(); return process.exitCode; }).timeout(timeout, onTimeout: () { print('Expected server to shutdown'); process.kill(); }).then((result) { if (result != null && result != 0) { exitCode = result; } return true; }); https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... File pkg/analysis_server/lib/src/channel.dart (right): https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... pkg/analysis_server/lib/src/channel.dart:40: void close(); This should return a Future. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... pkg/analysis_server/lib/src/channel.dart:92: .transform(new _JsonConverter()) Interesting. Is this so you can handle JSON objects that span multiple "messages" from the socket? I didn't think about needing to consider that case. :-/ https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... pkg/analysis_server/lib/src/channel.dart:94: .asBroadcastStream(); Fancy! :) https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... pkg/analysis_server/lib/src/channel.dart:114: _socket.close(); Future close() => _socket.close(); I'm not sure, but I think you probably want to close responseStream and notificationSteam here too? https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... pkg/analysis_server/lib/src/channel.dart:142: socket.add(_JSON_ENCODER.convert(notification.toJson())); JSON.encode(notification.toJson()) https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... pkg/analysis_server/lib/src/channel.dart:147: socket.add(_JSON_ENCODER.convert(response.toJson())); Ditto. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... pkg/analysis_server/lib/src/channel.dart:174: const _JSON_ENCODER = const JsonEncoder(null); You can ditch these and just use JSON. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... File pkg/analysis_server/lib/src/protocol.dart (right): https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... pkg/analysis_server/lib/src/protocol.dart:268: String id = json[Response.ID]; Embrace the "var". :) https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... pkg/analysis_server/lib/src/protocol.dart:271: } This is a personal preference, but I'd make this a one-liner. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... File pkg/analysis_server/test/channel_test.dart (right): https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/channel_test.dart:9: import 'mocks.dart'; Nit, but we tend to put local imports after "package:" ones. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/channel_test.dart:29: class ChannelTest { Why make a class for this? https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/channel_test.dart:45: notificationsReceived = new List(); new List() => [] https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/channel_test.dart:47: // Allow multiple listeners on server side for testing ".". https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/channel_test.dart:50: server.listen((data) => requestsReceived.add(data)); server.listen(requestsReceived.add) Ditto below. Yay for implicit closurization! https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/channel_test.dart:64: }); This might do the right thing because responseStream is broadcast, but I've run into problems using .first on a stream in tests. Nathan would know more here. I would style it like: static Future invalidJsonToClient() { socket.twin.add('{"foo":"bar"}'); server.sendResponse(new Response('myId')); return client.responseStream .first .timeout(new Duration(seconds: 1)) .then((response) { expect(response.id, equals('myId')); expectMsgCount(0, 1, 0); }); } https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/channel_test.dart:101: .then((_) {expectMsgCount(0, 1, 1);}); Use => here or make it a multi-line method if you want to use {}. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/channel_test.dart:123: expect(requestsReceived.first.runtimeType, equals(Request)); Do: requestsReceived.first, new InstanceOf<Request>() or just: requestsReceived.first is Request, isTrue https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/channel_test.dart:142: static void expectMsgCount(requestCount, responseCount, notificationCount) { I think the calls to this would be easier to read if you made these named parameters. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... File pkg/analysis_server/test/mocks.dart (right): https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/mocks.dart:14: class MockSocket<T> implements WebSocket { I wonder if you can extend Stream here too and have this class just be a Stream, instead of having to implement all of the Stream methods and forward? I haven't tried to see how well subclassing it works. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/mocks.dart:18: final JsonDecoder jsonDecoder = const JsonDecoder(null); You can probably just using the top-level "JSON" object in dart:convert instead of storing your own instances here. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/mocks.dart:26: MockSocket socket2 = new MockSocket(); I prefer using "var" for locals. Fortunately, there's this really nice analyzer than can infer the type from the initializer. :) https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/mocks.dart:47: Stream<T> where(bool test(T)) => stream.where(test); If you're going to forward these, it feels like you should forward all of them. It would be nice if there was a forwarding mixing for these. The collection package has something similar for the sync collection types.
On 2014/03/04 01:03:44, Bob Nystrom wrote: > I think the semantics here are OK, but I'm not an expert on streams. Added > Nathan because he knows them better than I do. > > Also, as usual, I was unable to resist commenting on style. I was hoping you would. Much appreciated! > But overall, this looks pretty idiomatic to me. > > If you find yourself writing more complex tests than these, I encourage you to > look at the scheduled_test package. It makes writing asynchronous test > expectations *much* nicer. Interesting. Could not find any articles, but looks like there are many pub tests using this that I can dig into and see how its used. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... File pkg/analysis_server/lib/src/analysis_manager.dart (right): https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... pkg/analysis_server/lib/src/analysis_manager.dart:121: channel.close(); On 2014/03/04 01:03:45, Bob Nystrom wrote: > This should probably be async too: > > return channel.close().then((_) => false); Good point. Updated close() to return a Future and used it here. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... pkg/analysis_server/lib/src/analysis_manager.dart:142: }); On 2014/03/04 01:03:45, Bob Nystrom wrote: > The logic looks good here, but I would format it like: > > var request = new Request('0', ServerDomainHandler.SHUTDOWN_METHOD); > var timeout = new Duration(seconds: 2); > return channel.sendRequest(request).timeout(timeout, onTimeout: () { > print('Expected shutdown response'); > }).then((response) { > channel.close(); > return process.exitCode; > }).timeout(timeout, onTimeout: () { > print('Expected server to shutdown'); > process.kill(); > }).then((result) { > if (result != null && result != 0) { > exitCode = result; > } > return true; > }); I find the wrapped style makes it harder to read the futures chain. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... File pkg/analysis_server/lib/src/channel.dart (right): https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... pkg/analysis_server/lib/src/channel.dart:40: void close(); On 2014/03/04 01:03:45, Bob Nystrom wrote: > This should return a Future. Good point. Done! https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... pkg/analysis_server/lib/src/channel.dart:92: .transform(new _JsonConverter()) On 2014/03/04 01:03:45, Bob Nystrom wrote: > Interesting. Is this so you can handle JSON objects that span multiple > "messages" from the socket? I didn't think about needing to consider that case. > :-/ It allows the stream to decide what characters in the stream comprise a chunk of JSON. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... pkg/analysis_server/lib/src/channel.dart:114: _socket.close(); On 2014/03/04 01:03:45, Bob Nystrom wrote: > Future close() => _socket.close(); Good point. Done. > I'm not sure, but I think you probably want to close responseStream and > notificationSteam here too? They are all interconnected streams, so closing the socket will close the others. I added a test to assert this. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... pkg/analysis_server/lib/src/channel.dart:142: socket.add(_JSON_ENCODER.convert(notification.toJson())); On 2014/03/04 01:03:45, Bob Nystrom wrote: > JSON.encode(notification.toJson()) Done. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... pkg/analysis_server/lib/src/channel.dart:147: socket.add(_JSON_ENCODER.convert(response.toJson())); On 2014/03/04 01:03:45, Bob Nystrom wrote: > Ditto. Done. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... pkg/analysis_server/lib/src/channel.dart:174: const _JSON_ENCODER = const JsonEncoder(null); On 2014/03/04 01:03:45, Bob Nystrom wrote: > You can ditch these and just use JSON. Done. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... File pkg/analysis_server/lib/src/protocol.dart (right): https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... pkg/analysis_server/lib/src/protocol.dart:268: String id = json[Response.ID]; On 2014/03/04 01:03:45, Bob Nystrom wrote: > Embrace the "var". :) Good point. The value could be any object. I've updated this code to use var and guard against non-String. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... pkg/analysis_server/lib/src/protocol.dart:271: } On 2014/03/04 01:03:45, Bob Nystrom wrote: > This is a personal preference, but I'd make this a one-liner. I might as well, but I'm trying to be consistent with the rest of this code base. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... File pkg/analysis_server/test/channel_test.dart (right): https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/channel_test.dart:9: import 'mocks.dart'; On 2014/03/04 01:03:45, Bob Nystrom wrote: > Nit, but we tend to put local imports after "package:" ones. Done. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/channel_test.dart:29: class ChannelTest { On 2014/03/04 01:03:45, Bob Nystrom wrote: > Why make a class for this? It groups the tests. We will have more tests for this library but focused on other classes. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/channel_test.dart:45: notificationsReceived = new List(); On 2014/03/04 01:03:45, Bob Nystrom wrote: > new List() => [] Done. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/channel_test.dart:47: // Allow multiple listeners on server side for testing On 2014/03/04 01:03:45, Bob Nystrom wrote: > ".". Done. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/channel_test.dart:50: server.listen((data) => requestsReceived.add(data)); On 2014/03/04 01:03:45, Bob Nystrom wrote: > server.listen(requestsReceived.add) > > Ditto below. Yay for implicit closurization! Nice! Done. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/channel_test.dart:64: }); On 2014/03/04 01:03:45, Bob Nystrom wrote: > This might do the right thing because responseStream is broadcast, but I've run > into problems using .first on a stream in tests. Nathan would know more here. > > I would style it like: > > static Future invalidJsonToClient() { > socket.twin.add('{"foo":"bar"}'); > server.sendResponse(new Response('myId')); > return client.responseStream > .first > .timeout(new Duration(seconds: 1)) > .then((response) { > expect(response.id, equals('myId')); > expectMsgCount(0, 1, 0); > }); > } The modified indentation makes it less obvious that the 2 statements are inside the then block. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/channel_test.dart:101: .then((_) {expectMsgCount(0, 1, 1);}); On 2014/03/04 01:03:45, Bob Nystrom wrote: > Use => here or make it a multi-line method if you want to use {}. Good point. Done. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/channel_test.dart:123: expect(requestsReceived.first.runtimeType, equals(Request)); On 2014/03/04 01:03:45, Bob Nystrom wrote: > Do: > > requestsReceived.first, new InstanceOf<Request>() > > or just: > > requestsReceived.first is Request, isTrue Good point. Done. Sure would be nice if the matcher defaulted to isTrue so that you could write expect(requestsReceived.first is Request) https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/channel_test.dart:142: static void expectMsgCount(requestCount, responseCount, notificationCount) { On 2014/03/04 01:03:45, Bob Nystrom wrote: > I think the calls to this would be easier to read if you made these named > parameters. I like it. Done.
Message was sent while issue was closed.
Committed patchset #4 manually as r33330 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... File pkg/analysis_server/lib/src/analysis_manager.dart (right): https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... pkg/analysis_server/lib/src/analysis_manager.dart:121: channel.close(); On 2014/03/04 01:03:45, Bob Nystrom wrote: > This should probably be async too: > > return channel.close().then((_) => false); Also below. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... File pkg/analysis_server/lib/src/channel.dart (right): https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... pkg/analysis_server/lib/src/channel.dart:92: .transform(new _JsonConverter()) On 2014/03/05 15:32:04, danrubel wrote: > On 2014/03/04 01:03:45, Bob Nystrom wrote: > > Interesting. Is this so you can handle JSON objects that span multiple > > "messages" from the socket? I didn't think about needing to consider that > case. > > :-/ > > It allows the stream to decide what characters in the stream comprise a chunk of > JSON. This seems strange to me. I believe web sockets guarantee that message boundaries will be preserved, so it seems like it would make for a much simpler protocol overall to just guarantee that each message is an individually valid JSON object. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... File pkg/analysis_server/test/channel_test.dart (right): https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/channel_test.dart:29: class ChannelTest { On 2014/03/05 15:32:04, danrubel wrote: > On 2014/03/04 01:03:45, Bob Nystrom wrote: > > Why make a class for this? > > It groups the tests. We will have more tests for this library but focused on > other classes. You're already using [group] for this. If you need coarser-grained grouping, I find it's cleaner to use multiple test libraries. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/channel_test.dart:64: }); On 2014/03/04 01:03:45, Bob Nystrom wrote: > This might do the right thing because responseStream is broadcast, but I've run > into problems using .first on a stream in tests. Nathan would know more here. With broadcast streams, it's important that you subscribe to them synchronously as soon as they're created, since otherwise they can drop events on the floor. In this case you do so, so you should be fine.
Message was sent while issue was closed.
https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... File pkg/analysis_server/test/channel_test.dart (right): https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/channel_test.dart:123: expect(requestsReceived.first.runtimeType, equals(Request)); On 2014/03/05 15:32:04, danrubel wrote: > On 2014/03/04 01:03:45, Bob Nystrom wrote: > > Do: > > > > requestsReceived.first, new InstanceOf<Request>() > > > > or just: > > > > requestsReceived.first is Request, isTrue > > Good point. Done. > > Sure would be nice if the matcher defaulted to isTrue so that you could write > > expect(requestsReceived.first is Request) It used to, but expect has optional named parameters and Dart doesn't let you mix optional positional and option named parameters. :(
Message was sent while issue was closed.
Sorry for the delay Nathan and thanks for the comments. I'm finally having time to get back to this. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... File pkg/analysis_server/lib/src/analysis_manager.dart (right): https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... pkg/analysis_server/lib/src/analysis_manager.dart:121: channel.close(); On 2014/03/05 20:16:39, nweiz wrote: > On 2014/03/04 01:03:45, Bob Nystrom wrote: > > This should probably be async too: > > > > return channel.close().then((_) => false); > > Also below. Done. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... File pkg/analysis_server/lib/src/channel.dart (right): https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/lib/... pkg/analysis_server/lib/src/channel.dart:92: .transform(new _JsonConverter()) On 2014/03/05 20:16:39, nweiz wrote: > On 2014/03/05 15:32:04, danrubel wrote: > > On 2014/03/04 01:03:45, Bob Nystrom wrote: > > > Interesting. Is this so you can handle JSON objects that span multiple > > > "messages" from the socket? I didn't think about needing to consider that > > case. > > > :-/ > > > > It allows the stream to decide what characters in the stream comprise a chunk > of > > JSON. > > This seems strange to me. I believe web sockets guarantee that message > boundaries will be preserved, so it seems like it would make for a much simpler > protocol overall to just guarantee that each message is an individually valid > JSON object. Good to know. Perhaps misunderstanding on my part. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... File pkg/analysis_server/test/channel_test.dart (right): https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/channel_test.dart:29: class ChannelTest { On 2014/03/05 20:16:39, nweiz wrote: > On 2014/03/05 15:32:04, danrubel wrote: > > On 2014/03/04 01:03:45, Bob Nystrom wrote: > > > Why make a class for this? > > > > It groups the tests. We will have more tests for this library but focused on > > other classes. > > You're already using [group] for this. If you need coarser-grained grouping, I > find it's cleaner to use multiple test libraries. Good point. https://codereview.chromium.org/185313002/diff/40001/pkg/analysis_server/test... pkg/analysis_server/test/channel_test.dart:64: }); On 2014/03/05 20:16:39, nweiz wrote: > On 2014/03/04 01:03:45, Bob Nystrom wrote: > > This might do the right thing because responseStream is broadcast, but I've > run > > into problems using .first on a stream in tests. Nathan would know more here. > > With broadcast streams, it's important that you subscribe to them synchronously > as soon as they're created, since otherwise they can drop events on the floor. > In this case you do so, so you should be fine. Good point. https://codereview.chromium.org/195463004 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
