Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add server_client example #5

Merged
merged 1 commit into from
Oct 7, 2018
Merged

Add server_client example #5

merged 1 commit into from
Oct 7, 2018

Conversation

torkleyy
Copy link
Contributor

@torkleyy torkleyy commented Oct 6, 2018

This is a stupid example, yes. I'm probably doing something wrong, because I noticed two issues:

  • messages take very long (~ 1 second) until they're shown. probably some blocking going on somewhere?
  • when there is no server and I send a message with the client, I don't get any error message (nothing happens)

@torkleyy
Copy link
Contributor Author

torkleyy commented Oct 6, 2018

Not meant to be merged

@TimonPost
Copy link
Owner

A lol, I also worked on examples but now I see you also did some. I noticed something too that it took long before an message was received further investigation needed.

@TimonPost
Copy link
Owner

I did some test to monitor how long it took to send a message and it took 1 second. So there is some bug somewhere :)

test results:

= Message took 72.982µs to send =
Moving to lat: 10.555, long: 10.55454, alt: 1.3
=Message took 990.276878ms to send =
Moving to lat: 5.4545, long: 3.344, alt: 1.33
= Message took 990.114451ms to send =
Received text: "Some information"

After debugging I found that create_connection_if_not_exists method takes 0.5 - 0.9 seconds to execute.

And found out that acquiring the lock on connections is taking to long in create_connection_if_not_exists method.

  let mut lock = self
            .connections
            .write()
            .map_err(|_| NetworkError::AddConnectionToManagerFailed)?;

Copy link
Contributor Author

@torkleyy torkleyy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be merged now. Should I add a comment to the example that it doesn't reflect the use-cases of this crate?

Copy link
Owner

@TimonPost TimonPost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice chat example, maybe not ideal for UDP but still fun example!. Further nice fix for that bug with the lock. I have tested how fast it was but it was way faster than before so looks good to me.

examples/server_client.rs Outdated Show resolved Hide resolved
examples/server_client.rs Outdated Show resolved Hide resolved
examples/server_client.rs Show resolved Hide resolved
stdin.read_line(&mut s)?;
let line = s.replace(|x| x == '\n' || x == '\r', "");

socket.send(Packet::new(server, line.clone().into_bytes()))??;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

???

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, those are result wrapped results, most likely for non-blocking stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I'm not sure if the API is designed right here.

examples/server_client.rs Show resolved Hide resolved
examples/server_client.rs Outdated Show resolved Hide resolved
src/net/socket_state.rs Outdated Show resolved Hide resolved
src/net/socket_state.rs Outdated Show resolved Hide resolved
This was referenced Oct 6, 2018
examples/server_client.rs Show resolved Hide resolved
examples/server_client.rs Show resolved Hide resolved
@torkleyy
Copy link
Contributor Author

torkleyy commented Oct 7, 2018

Ready for merge.

* Add printlns, use SocketAddr::parse
* Rename s to s_buffer
@TimonPost TimonPost merged commit b8cc354 into TimonPost:master Oct 7, 2018
@torkleyy torkleyy deleted the exa branch October 7, 2018 09:08
@TimonPost TimonPost mentioned this pull request Oct 22, 2018
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants