Add notifications through websockets #2004
Conversation
Removed because the cost of reuse a ConnectionInfo instance is not a real advantage.
| const size_t WSConstants::DataSize = 128; | ||
| // Ok, this is can be a macro, but I prefer | ||
| // to be consistent | ||
| const std::string WSConstants::ProtocolName = "orion-ws"; |
There was a problem hiding this comment.
Maybe we have talked about this in the past but I don't rembember... what is the purpose of this "orion-ws"?
There was a problem hiding this comment.
Is the protocol name used for the clients to connect with our service
There was a problem hiding this comment.
While we decide the right name ( @jmcanterafonseca , any suggestion?) maybe we should use something that clearly specifies that the name is temporal, e.g. orion-ws-experimental.
There was a problem hiding this comment.
well, the sub-protocol, application level protocol, is an optional part in the Javascript API.
Having said that, wouldn't it be 'ngsiv2-json' a proper name for the sub-protocol? Maybe in the future we implement 'ngsiv2-text' or 'ngsiv2-binary' ...
By the way, there is a registry of subprotocols
https://www.iana.org/assignments/websocket/websocket.xml#subprotocol-name
if we say 'orion-xxx' it can be less standard if others decide to implement this as we have done for Orion.
There was a problem hiding this comment.
Ok, changet it in c824a6c
Remove subscription from DB when websocket connection is closed
| if (pos == std::string::npos) | ||
| return false; | ||
|
|
||
| char buff[25]; |
There was a problem hiding this comment.
I'd suggest to use (in globals/limits.h)
#define MAX_LENGTH_SUBID 24
and size the buff variable with MAX_LENGTH_SUBID + 1. Otherwise, the raw 24 and 25 may be a bit obscure.
(const is also ok if you prefer that to #deffine)
|
LGTM (passing the ball to @kzangeli for final LGTM + merge + delete branch) |
| if (written == -1) | ||
| return 1; | ||
| } | ||
|
|
There was a problem hiding this comment.
A few things about this loop ...
To detect -1 from lws_write you need a separate variable, the accumulating 'written' cannot be used:
int bytes = lws_write(it->second, (p + written), strlen(msg), LWS_WRITE_TEXT);
if (bytes == -1)
return 1;
written += bytes
AND, the third parameter to lws_write, the size of the buffer to be sent must adapt to the number of bytes already written, and strlen should not be calculated in each loop, so:
int bytes = lws_write(it->second, (p + written), msg_size - written, LWS_WRITE_TEXT);
Also (this is just my opinion), isn't &p[written] easier to understand than p + written ?
There was a problem hiding this comment.
Fixed in 51652e9
and about to use &p[written] or p + writter, I prefer the second option, because is really a pointer not an array.
|
LGTM |
Implements #1669