Hot answers tagged

30

Checking TCP ports and multi-threading Your performance should be fine for a sequential program. Another way to check whether the TCP port is open is by just sending the initial SYN and wait for the ACK. That way, you don't have to do the complete TCP handshake. However, that also means that you have to implement that part of the TCP protocol yourself. Also,...


26

Networking You have done the classic thing that all people new to TCP do of assuming that whatever NetworkStream.Read gives you will be some meaningful chunk of data: this is not guaranteed, and only 'works' because you are sending tiny packages locally. Basically, if you want this to be a general-purpose and reusable system for sending discrete messages (i....


18

Design This is a very thin C like wrapper around sockets. In my opinion there are a lot of style changes that are need to make this good C++ or usable by modern C++ library. Main issues: Two phase construction Error Codes escape public interface (use exception) You member variables have state not related to the object after construction. Your use of ...


18

Networking Concerns I'm going to assume that stream is a NetworkStream in the Server. It is really really important that you understand that stuff which 'just happens to work' with a 'normal' stream, won't work with a NetworkStream, which also doesn't provide a 'complete' Stream implementation (being non-seeking and such it can't). Your code currently could ...


13

This is an ugly old pattern. Why don't just try the newer async/await? You already use Task in your tests anyway. In order to implement it the awaitable way you just need to use a different API, in this case AcceptTcpClientAsync and build everything on top of it. With the CancellationToken you can now better control the server. public class TcpServer : ...


13

This won't be comprehensive review/ramble, because I'm hungry; however, it is nice to see fairly comprehensive inline documentation on the public API! General Networking Commentary Because you asked if there are better ways of doing this, here is some high-level discussion that doesn't relate so much to your precise code. I've written code like this dozens ...


12

catch (Exception) { Reconnect(); } The same pattern can be found in almost every method. Are you not interested in what happened? No logging or anything? Just ignore the exception? _pingTimer = new Timer { Interval = 2000 }; Thread.Sleep(500); Hardcoded numbers? No settings? What if this won't work and you have to tune it? You'll have to update all ...


11

A few notes: As Jamal noted in the comments, your definitions of STDIN and STDOUT are somewhat useless. You can see here that using the given STDIN_FILENO and STDOUT_FILENO are already given those values. To counter your claim in the comments that they help readability, I would argue that they actually clutter up your code and should be removed. Declaring ...


10

Are you sure that &(int) { 1 } does what you want it to do? I'm not sure it does. Using malloc()/strcpy() is probably better handled with strdup() (which you've used elsewhere, so you're already okay with that). In webserver_handle_connection, you're using malloc() to allocate space for an unsigned long. Why? Declare a normal local variable, and pass ...


10

Review This review handles readability metrics and C# conventions only and should provide insights to reach some of your goals. Goals: learn C#, improve coding style, create good to read code There are a couple of variants how to name instance variables. The most common one is to use an underscore as prefix and camel-case the name; _client, _listener, ...


9

There are a number of things in here which could be a problem, and a number of other things which are a problem.... Ephemerals First up, you may just be running in to resource limitations. A TCP based computer has just 64K ports available for sockets. The first 1K are reserved for root, and the remainder are available for programs. You may think you have >...


9

Detect closed connection 1 select promises that a read wouldn't block. This is a case when there are data, but this is also a case that the remote end is closed. In this case read immediately returns 0. Without testing for len > 0 in transfer end up in an infinite empty select/read cycle. Detect closed connection 2 The remote may close connection while ...


9

apart from what @t3chb0t has recommended, Reconnect should implement something like exponential back off or at least increase the sleep time also your IsConnected login can be improved TcpClient has a Connected property which you should check first. you only need to use Send to check for half dead connections which can either be handled with your Ping timer ...


8

Not much to comment on, this program is pretty straightforward. A few notes: The user of the client has a lot of information to input. fprintf(stderr, "Usage: %s <Server IP> <Server Port> <Iterations> <Gap>\n", argv[0]); The more the user has to enter, the steeper the initial learning curve to use the program is. ...


8

Being that both Timer and SimpleTcpClient implement IDisposable, your class should also implement IDisposable and follow the disposable pattern. Do not call Initialize from your constructor. Constructors are for initializing state, not starting processing. Your calling code should call the constructor and then call Initialize. It would be handy if you ...


8

I don't understand the point of the IComponent / IComponentConsumer stuff. Nothing else in the large section of code you've posted uses them. Are you sure they're at the right layer? Also, with IComponentConsumer, I think the code falls into the trap of extending when it should compose. It seems to me that every class which implements the interface will ...


7

A couple of comments: Shortening code is not a goal - readability and maintainability is (which may, but not neccessarily, shorten it). The code mixes net code, graphics code, encoder code and UI code. Separating these into separate functions/classes will be very good. Just remember that (as a general rule), one function should do one thing. Always use ...


7

You should only use Task.Run to start threads, which you don't want to do when you're doing IO, at least directly. You should let the runtime make that decision. Also you need to make sure your tcpClient isn't already connected. There's also a tcpClient.ConnectAsync that retruns a Task so you should use that. Also you should never pass a ...


7

It looks good, though some parts are too tightly packed for my taste. This is certainly subjective, but I would like to have some more blank lines in a few places, to better separate the code into distinct "paragraphs". Compiler warnings: Building your source file with: clang -std=c89 -Wall -Wextra -pedantic test.c produced a couple warnings: test.c:57:10:...


7

Cryptic variable names I had a hard time reading your code due to all of the short variable names such as cs, ss, scl, cal, ca, etc. It would be very helpful if you used descriptive names. Unsafe argument passing to threads Right now, your threads require two arguments, the file descriptors scl and fcl. The threads are getting these arguments from global ...


7

These are good questions! 1. You should indeed have the copy constructor & assignment operator deleted. The "rule of five" tells you to specifically define a copy c'tor and assignment operator - but it doesn't tell you that you have to make the available. It is a perfectly valid choice to decide to not allow your object to be copied or non-move-...


6

How about using Generate as prefix? GenerateMonitorPacket GenerateControlTempPacket GeneratePutPeakInfoPacket GenerateGetPeakInfoPacket Alternatively, you can also create a separate class for each type of packet generator. You've only shown the code to create the packet for Monitor, so I don't know how complex the code for the other packets is, but ...


6

As the latecomer to the party, I'll take it from V3... First, async void should only be used for event handlers. I'd much rather see Start return a Task representing the listening loop. For a simple example, you don't need to do any cleanup at all. Once your app exits, the OS will clean up after it. Doing cleanup just before application exit is just a ...


6

You are holding on to some IDisposable resources that you shouldn't be, which will impact scalability, GC pressure and likely performance. Here's the augmented Listener class: internal sealed class Listener : IDisposable { private readonly int port; private readonly TcpListener tcpListener; private readonly IList<Client> clients = new ...


6

dead code should be removed the indention is horrible byte[] StreamMessage = new byte[9632*2]; try { socket = peerListener.AcceptSocket(); Thread.Sleep(500); MessageLength = socket.Receive(StreamMessage, 0, StreamMessage.Length, SocketFlags.None); } should be look like byte[] StreamMessage = new byte[9632*2]; ...


6

A few of the class names are unclear. ServerListener sounds like it listens for some action by the server. Instead, it is the code that runs on the server listening for connections from clients. Authentication doesn't do any authentication, it is just a user's login credentials. Most of your classes are Runnable, but they encapsulate the thread that they ...


6

After creating a socket make it non-blocking. Then call connect(). If connect returns 0 then the connection has been established and you can use it already. If it retuned -1 and errno is equal to EINPROGRESS then you need to wait until it becomes write-ready using select() or poll(). When it becomes write-ready use getsockopt(sock, SOL_SOCKET, SO_ERROR, ...) ...


6

This may not be useful for accounting for invalid command line arguments: assert(args == 3); It's more common to inform the user of the correct arguments upon failure. This message should be printed to stderr and the program should terminate with some valid error value. Here's an example of this: if (argc != 3) { fprintf(stderr, "usage: %s arg1 arg2"...


6

Return value handling There are a few places with potential problems involving return values: resolve() rv = resolve(host); if (rv < 0) return print_error(rv); This seems ok until you look at resolve(): int resolve(char *host) { struct addrinfo hints, *servinfo; struct in_addr addr; char *addr_tmp; int rv; memset(&...


6

Async/Await Async/await's main advantage is that you no longer need to use ContinueWith() to chain tasks, making the code much cleaner. Your code seems to more often favor the use of ContinueWith(). A good book or tutorial on this will help you refactor that part of the code. For example, this code: public Task<int> DoSomethingAsync() { return ...


Only top voted, non community-wiki answers of a minimum length are eligible