hyper-prog / hasses

Hyper's Asynchronous Server Sent event (SSE) notification Server
http://hyperprog.com/hasses/index.html
GNU General Public License v2.0
24 stars 9 forks source link

Configurable delimiter #7

Closed mterron closed 2 years ago

mterron commented 2 years ago

In https://github.com/hyper-prog/hasses/issues/6#issuecomment-752377632 you've explained how the multiple topic subscription works.

Can you make the delimiter configurable instead of the fixed string ;? Not being able to use ; in a message is quite restrictive in the face of JSON. Alternatively maybe use a less common character as separator || which is commonly use to express an OR relation?

Implementation wise, strsep and strtok are built in functions that do what you want. Why did you do it manually, performance?

hyper-prog commented 2 years ago

Hi! Sorry for late, I will check my subscriptions because github didn't notify me...

I don't mind if the delimiter is configurable, but I think the choices must be limited to some TESTED case. I don't want to anybody set the delimiter to : , because it used by sse protocol. I think we should test some characters as delimiter, and only accept that characters as delimiter.

strtok,strsep: So I don't like strtok because it maintains an internal state. If you use more strtok simultaneously, (because you need to tokenize the tokenized parts too), it can mess up the internal state. The strsep is same. Ok you can add strsep here, no problem. But later you add it to chat.c code later to tokenize something else. The outer strsep state will messed. That's why I avoid using it. Don't afraid leaving the original code that way. It works without any function call, it works independently of the other codes and fast.

Notice about code analysis: It is good things, but I don't like to apply every suggestion made. For example it unnecessary to replace strcpy(some_string,"") to strlcpy(some_string,"",1) The empty string is written to the memory here. We know, the size, it's not changing. That cases where the second parameter is a variable or even a larger string it's Ok to change to strlcpy.

mterron commented 2 years ago

As far as I understand it (not a C programmer so bear with me). Every time you call strtok (or strsep) with a new string it re-initializes the internal state. It only maintains it if you all it with NULL as the string to tokenise.

If you need to reuse the original string, you need to make a copy, but that is not the case here is it?

hyper-prog commented 2 years ago

Yes, it re-initialize the internal state every time you call with new string. So you have a string: first=red;second=blue;third=green You call strsep to the whole string with ; When you call strsep to first=red with =, it will mess up the state of the first strsep.

As far I remember that's why I avoid using it.

mterron commented 2 years ago

I see what you mean! You need to copy first=red to a new string and then tokenise that.

hyper-prog commented 2 years ago

Yes exactly! But I do not want to copy every parts every time I tokenise something new part. I did the whole work in place without unnecessary copies.

mterron commented 2 years ago

I guess you can cherrypick https://github.com/hyper-prog/hasses/pull/8/commits/71e69252c26d7b8b9853c953869f60b91d161c9c and drop all the rest.

hyper-prog commented 2 years ago

I have already merged 3 of your commits, including the configurable delimiter. I will check out the rest later if I have more time. Anyway, Thank you the contributing!

For interest only: Do you use hasses on public site, or experiments only? What is your experience with higher user numbers, if any?

mterron commented 2 years ago

Hi, it was an experiment only, haven't actually had to use it in anger. Just saw I could contribute a bit cause it looks interesting. :)

hyper-prog commented 2 years ago

Thanks!