Implement logger adapter #239

kstrafe · 2019-09-07T21:36:24Z

No description provided.

Since the protocol specifies the use of 16 bit counters, it's best to reflect these in the API as well.

Turn arranging usize into u16

The expected index is a u16 at the protocol level, but the arranging implementation for orderingstream does not take this into account. This caused sending over 2**16 ordered packets to simply default to the `None` case and get ignored. This patch solves that issue.

Previously we would only store packets with indices higher than the expected index, this is obviously not going to give us good results when we receive packets 0..100 while we are currently expecting index 65500. What this patch does is that it stores all incoming packets with the condition: expected < index < expected + u16::max/2 With wrapping add this becomes slightly more complex but the gist is the same.

Fix ordering arranging handler for >65536 packets

The previous patches would erroneously skip the value 32767 and drop the next packet (32768). This patch and test ensures that this doesn't happen.

Sending over 65536 packets would previously just saturate the top_index which caused it to not accept any more packets. This patch shortens the sequence acceptance by half but always loops around the max u16 value. This means that at top_index = 0, all sequenced packets 0-32768 are accepted, while at top_index = 50000, all sequenced packets 50000-65535 and 0-17233 will be accepted.

…ays_increase Ensure that `self.remote_ack_sequence_num` is always increasing

crossjob for building and testing laminar

…#234) The connection is disconnected if we have N packets-in-flight simultaneously. Under normal usage we expect packets to be acked regularly so that our packets-in-flight size is relatively small.

src/net/socket.rs

TimonPost · 2019-09-08T18:00:23Z

src/log.rs

+
+/// Holds a handle to a formatter function while implementing the [fmt::Display] trait.
+pub struct Displayer {
+    data: Arc<dyn Fn(&mut ::std::fmt::Formatter) -> ::std::fmt::Result + Send + Sync>,


Is Arc realy needed here

Arc is most general, for instance if you want to send your log messages to another thread.
To be honest I'd prefer if we can do it completely without dynamic allocation but that doesn't allow for arbitrary data to be sent (because sized information is required for that) - data sent through a move |f| format!(f, "{}", x) requires it because of the arbitrary size of x.

Is there an actual need/use-case of wanting to do that today? I prefer not using the most general solution unless we have a hard requirement that it is needed.

TimonPost · 2019-09-08T18:02:27Z

src/log.rs

+pub(crate) struct DefaultLogger;
+
+impl LaminarLogger for DefaultLogger {
+    fn trace(&self, _: Displayer) {}


don't we want to implement this?

I'm not sure. I think the library should be silent if not configured to log. Doing default logging could annoy a user. It's perhaps best to inform of misuse via socketevents (for instance the proposed Disconnect(addr, reason) event or eventual panics.

Right, seems like a good point, just moving decisions over to the user. We might implement a logger using log and move it under the tester feature flag. That would allow us to debug if we provide our tester flag

I honestly think that the default logger should maintain its current behavior but we can also provide a SilentLogger which is this implementation you can configure when setting up your connection.

TimonPost · 2019-09-14T11:23:19Z

There are some errors in the code

TimonPost · 2019-09-14T11:23:52Z

@jstnlef can you check this PR as well before merging?

jstnlef · 2019-09-14T21:22:40Z

src/log.rs

+
+/// Holds a handle to a formatter function while implementing the [fmt::Display] trait.
+pub struct Displayer {
+    data: Arc<dyn Fn(&mut ::std::fmt::Formatter) -> ::std::fmt::Result + Send + Sync>,


Is there an actual need/use-case of wanting to do that today? I prefer not using the most general solution unless we have a hard requirement that it is needed.

jstnlef · 2019-09-14T21:26:44Z

src/log.rs

+pub(crate) struct DefaultLogger;
+
+impl LaminarLogger for DefaultLogger {
+    fn trace(&self, _: Displayer) {}


I honestly think that the default logger should maintain its current behavior but we can also provide a SilentLogger which is this implementation you can configure when setting up your connection.

jstnlef · 2019-09-14T21:31:00Z

src/net/socket.rs

@@ -142,7 +142,10 @@ impl Socket {
            match self.recv_from(time) {
                Ok(UdpSocketState::MaybeMore) => continue,
                Ok(UdpSocketState::MaybeEmpty) => break,
-                Err(e) => error!("Encountered an error receiving data: {:?}", e),
+                Err(e) => error!(
+                    self.config.logger,


Having to specify this each time is a bit annoying. Is there anything clever we could do about that?

It'd be possible to make a trait that Self implements that returns a logger, but that's about it.

Kevin R. S and others added 17 commits August 11, 2019 14:58

Add missing asserts (TimonPost#226)

6a86f50

Implement heartbeat functionality (TimonPost#224)

58630ed

Turn arranging usize into u16

3cd0310

Since the protocol specifies the use of 16 bit counters, it's best to reflect these in the API as well.

Merge pull request TimonPost#228 from BourgondAries/arrange-u16

d8f8bd1

Turn arranging usize into u16

ordered: Fix spelling

68bdf8f

error: Add dyn to trait object

1280d6f

ordering: Change the ordered stream expected index to default to 0

ce03a18

Merge pull request TimonPost#229 from BourgondAries/ordered-fail-2

d93fc01

Fix ordering arranging handler for >65536 packets

ensure that self.remote_ack_sequence_num is always increasing

edf29f1

Fix exact indexing values for half-window calculations (TimonPost#230)

d583916

The previous patches would erroneously skip the value 32767 and drop the next packet (32768). This patch and test ensures that this doesn't happen.

Merge pull request TimonPost#233 from jstnlef/remote_ack_seq_must_alw…

1d8917d

…ays_increase Ensure that `self.remote_ack_sequence_num` is always increasing

Initial commit build and test script

5e2add0

crossjob for building and testing laminar

Removed example config

118ae6a

Disconnect the connection after sending N un-acked packets (TimonPost…

a67ddbd

…#234) The connection is disconnected if we have N packets-in-flight simultaneously. Under normal usage we expect packets to be acked regularly so that our packets-in-flight size is relatively small.

kstrafe force-pushed the logger-adapter branch from b6da88f to c02c6a8 Compare September 7, 2019 22:00

TimonPost reviewed Sep 9, 2019

View reviewed changes

kstrafe force-pushed the logger-adapter branch 2 times, most recently from 36d8ff8 to 8314d46 Compare September 9, 2019 19:59

Implement logger adapter

8314d46

TimonPost approved these changes Sep 14, 2019

View reviewed changes

TimonPost requested a review from jstnlef September 14, 2019 11:23

jstnlef reviewed Sep 14, 2019

View reviewed changes

fraillt mentioned this pull request Sep 24, 2019

WIP connection manager #246

Closed

jstnlef force-pushed the master branch from 8acde72 to 2758f32 Compare October 12, 2019 05:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement logger adapter #239

Implement logger adapter #239

kstrafe commented Sep 7, 2019

TimonPost Sep 8, 2019

kstrafe Sep 9, 2019

jstnlef Sep 14, 2019

TimonPost Sep 8, 2019

kstrafe Sep 9, 2019 •

edited

Loading

TimonPost Sep 10, 2019

jstnlef Sep 14, 2019

TimonPost commented Sep 14, 2019

TimonPost commented Sep 14, 2019

jstnlef Sep 14, 2019

jstnlef Sep 14, 2019

jstnlef Sep 14, 2019

kstrafe Sep 15, 2019

Implement logger adapter #239

Are you sure you want to change the base?

Implement logger adapter #239

Conversation

kstrafe commented Sep 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kstrafe Sep 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimonPost commented Sep 14, 2019

TimonPost commented Sep 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kstrafe Sep 9, 2019 •

edited

Loading