dhbaird / easywsclient

A short and sweet WebSocket client for C++
MIT License
741 stars 205 forks source link

cleaner code. #30

Closed pbplugge closed 10 years ago

pbplugge commented 10 years ago

Dear mister Baird,

I would like to help by cleaning up the code and make a neat object from the easy web socket. It would be a big change but IMHO a big improvement. Using it, debugging it and updating it and reading the code would be a lot easier. Using it would be a bit different though. Are you open for such a commit? It probably would be done in 3 days.

Regards

dhbaird commented 10 years ago

I'm definitely open to improvement. I can always review what you come up with, but if you can provide some more information, that would be helpful. Can you propose what the new interface is, and if there are any dependency changes? If the interface changes, it may be better to consider developing it under a "v2" branch while it gets settled out and to create a transition path to help people switch to it.

pbplugge commented 10 years ago

I propose a class only websocket, no namespaces and no websocket code outside the class. This means more websockets can be started by the application by various threads. I propose a hpp file with the class definition and function definitions. It is called easywsclient so it should be as easy to use as possible. I run into some bugs to that i want fixed. This does not run correctly with me as i don't receive hello3 on the server:

ws->send("hello1");
if (ws->getReadyState() != WebSocket::CLOSED) {
  ws->poll();
  ws->dispatch(handle_message);
}
ws->send("hello2");
if (ws->getReadyState() != WebSocket::CLOSED) {
  ws->poll();
  ws->dispatch(handle_message);
}
ws->send("hello3");

I get back to you soon with my version of the easywsclient. Weather you want to adopt it as v2 is up to you.

On Mon, Sep 1, 2014 at 3:52 PM, David Baird notifications@github.com wrote:

I'm definitely open to improvement. I can always review what you come up with, but if you can provide some more information, that would be helpful. Can you propose what the new interface is, and if there are any dependency changes? If the interface changes, it may be better to consider developing it under a "v2" branch while it gets settled out and to create a transition path to help people switch to it.

— Reply to this email directly or view it on GitHub https://github.com/dhbaird/easywsclient/issues/30#issuecomment-54061118.

dhbaird commented 10 years ago

@pbplugge, I did not understand all of your points and have the following questions, and hopefully clarification as well to how the library works:

  1. How would eliminating namespaces fix things? If you feel it's a hassle, just type using namespace easywsclient at the top of your code. Is there some other reason that you have for wanting to eliminate namespaces?
  2. Threading: It is possible right now for various threads to create websockets. For example, if you want to have 100 threads, each with their own websocket instance, that would be perfectly fine. If you observed otherwise, it is a bug, and please file a ticket for it.
  3. You need to call poll() (repeatedly, until the send buffer is emptied). After calling ws->send("hello3"), you must call poll() in order for it to be sent. Actual sending and receiving (over sockets) is handled in the poll() function. It is safe to call ws->poll() without needing to check WebSocket::CLOSED.

I can offer one small word of wisdom: breaking the problem into smaller pieces would be much easier to digest than a full rewrite and a change of interface (but I also don't want to discourage you if you feel there is a good need for such a full rewrite). Based on the observations above, it sounds partly like the documentation just needs some better examples.

There is a particular deficiency that you may be interested in seeing if you can help address, that I've written up here: #31.

Good luck. Looking forward to see what you come up with :)

pbplugge commented 10 years ago

Ok, thanks for your reply. I did not know about poll. As a simple user, if i call send, i would expect my message to be send without having to call another function. Tonight i will take a look at some other web socket implementations to see if there is a function call standard that i can take in consideration with my version.

On Tue, Sep 2, 2014 at 4:55 AM, David Baird notifications@github.com wrote:

@pbplugge https://github.com/pbplugge, I did not understand all of your points and have the following questions, and hopefully clarification as well to how the library works:

  1. How would eliminating namespaces fix things? If you feel it's a hassle, just type using namespace easywsclient at the top of your code. Is there some other reason that you have for wanting to eliminate namespaces?
  2. Threading: It is possible right now for various threads to create websockets. For example, if you want to have 100 threads, each with their own websocket instance, that would be perfectly fine. If you observed otherwise, it is a bug, and please file a ticket for it.
  3. You need to call poll() (repeatedly, until the send buffer is emptied). After calling ws->send("hello3"), you must call poll() in order for it to be sent. Actual sending and receiving (over sockets) is handled in the poll() function. It is safe to call ws->poll() without needing to check WebSocket::CLOSED.

I can offer one small word of wisdom: breaking the problem into smaller pieces would be much easier to digest than a full rewrite and a change of interface (but I also don't want to discourage you if you feel there is a good need for such a full rewrite). Based on the observations above, it sounds partly like the documentation just needs some better examples.

There is a particular deficiency that you may be interested in seeing if you can help address, that I've written up here: #31 https://github.com/dhbaird/easywsclient/issues/31.

Good luck. Looking forward to see what you come up with :)

— Reply to this email directly or view it on GitHub https://github.com/dhbaird/easywsclient/issues/30#issuecomment-54104739.

dhbaird commented 10 years ago

@pbplugge - not sure if this helps, but there is the W3C spec. I wish I had followed that more closely. That means, instead of a "dispatch()" method, the interface should have a "onmessage" setter. As for "poll()"- I'm open to ideas for improving it. I have thought about it and there are some reasons for why it exists, but I don't entirely like it.

pbplugge commented 10 years ago

I think you might like what i am making then. Spend another few good hours on it and it takes shape. Talk to you soon.

On Tue, Sep 2, 2014 at 2:46 PM, David Baird notifications@github.com wrote:

@pbplugge https://github.com/pbplugge - not sure if this helps, but there is the W3C spec http://www.w3.org/TR/websockets/. I wish I had followed that more closely. That means, instead of a "dispatch()" method, the interface should have a "onmessage" setter. As for "poll()"- I'm open to ideas for improving it. I have thought about it and there are some reasons for why it exists, but I don't entirely like it.

— Reply to this email directly or view it on GitHub https://github.com/dhbaird/easywsclient/issues/30#issuecomment-54145724.

pbplugge commented 10 years ago

Well, i am about done, i hope you will like it. I have no time now to commit today since i am not any good with git. I rather test it first coming days with my own software and commit on monday with some help.

I will create a json proxy today and tomorow for my version since i use that too and that is the format used with communication between websockets.

pbplugge commented 10 years ago

FYI. I just committed my code but for the past hours i am trying to make it work with my own application but it won't send the message correctly. That is to say, the server does not receive it. So it needs some more work.

pbplugge commented 10 years ago

My test application works but as soon as i migrate to my project it crashes. That looks to me like an uninitialized variable or memory leak.

My easywsclient disconnects immediately when either of these 2 commands runs:

(i changed char* cast to int but does not make a difference) setsockopt(sockfd, IPPROTO_TCP, TCP_NODELAY, &flag, sizeof(flag)); // Disable Nagle's algorithm

fcntl(sockfd, F_SETFL, O_NONBLOCK);

If i don't run these commands, the the connection stays alive untill the application quits. But of course i cannot communicate becouse these options are not set.

Hopefully someone has an idear on how to fix this.

pbplugge commented 10 years ago

Now these commands suddenly work.... easywsclient still not... disregard previous post i keep looking.

edit: now it disconnects on. int ret = send(sockfd, (char*) &txbuf[0], txbuf.size(), 0); return;

It does not disconnect on: int ret = send(sockfd, "test", 4, 0); return;

pbplugge commented 10 years ago

issue seems to be solved. when i set useMask on true it works. great, now i can go fast forward. if you like my version i'll commit.