Improve web socket implementation #194
Conversation
|
I'm sorry, but you'll have to explain to me what the actual problem is first. I don't see any corresponding issues reported, nor am I currently aware of problems in websocket handling. |
|
There is the problem that I discovered during prototyping standalone client. If header and body of Web-socket message are split between several IP packets, then previous implementation raise exception IncompleteRead |
|
I am very hesitant to touch the websocket implementation. It took a lot of iterations to get it to a point where it is running stable. How much testing did you do on your modification? There is no standalone client, nor has anybody so far expressed the need for it. The websocket of OpenWebRX is not an exposed or stable API and I won't really support the development of third-party software with it. It is an internal connection to be used by the included website, period. Does the websocket spec even allow splitting the header across multiple packets? |
|
I have tested it with official web-client and in different scenarios including long run (several hours), page refresh and connection drop. It is sad that nobody is interested in other client that is usable under moblie and embedded devices. WebSocket use TCP as transport and it is normal for TCP stream to be sliced to different IP packets. In many cases user of TCP socket API cannot guarantee that all data that were submitted with single write call will not be splitted to multiple IP pcakets and that all data form for single read call on receiver side. |
|
The problems with the websockets don't show up in long-running connections, they show up in different types of networks, dropped packets, congestions and clients that don't close their connection properly. Those are the scenarios that need to work, and they are hard to test. Well let's look at the current situation: The websocket isn't advertised as a public API anywhere. You probably have all the information about it from reverse engineering. Since it isn't an official API, it may change at any point in the future, and since it isn't an official API, I am not going to hesitate to do so. If you build on top of that, it is going to break. The first step that would be necessary, therefor, would be to standardize it, version it, document it, just to ensure some level of interoperability between different versions. I do not have the time to do this. The next thing is that once the API has been standardized, it will require a separate process to implement changes, which will slow down the development process. I am not ready to accept that either. So, all in all: It takes way too much time to do it properly. If you want to continue without doing it properly, that's fine with me, but don't expect me to support it. |
I understand, It was not an easiest task to understand how it works. Also during my experiments I have spotted IncompleteRead exception with current implementation with official web-client running on Firefox 82.0 (Arch Linux x86_64). So there is a problem that occurs very infrequently and I just have highlighted it with lwIP/ESP32 web-socket implementation during my experiments. I will continue with my experiments in any case. In worst case when it will be needed to maintain fork. |
|
I don't think a fork is a good idea. And that's coming from me, I took on this whole project by starting a fork. The difference is that there's still active development on the mainline this time. There's a lot of other open issues and feature requests that would profit a lot of users and would ease my mind instead of bogging me down even more, hence my irritation. I have seen requests for mobile use of OpenWebRX before, but for me, there simply isn't enough space on a mobile screen to make it work. If you wish to work on mobile though, I still think a responsive web page is the better way to go. Either way, it will take some time and focus to review these changes. I'm currently working on the connectors, so it may take some time. |
|
I agree with you, that for mobile use display size is a significant constraint. But in many cases user can sacrifice waterfall and use other controls. I think that Web interface should be because it is universal and it should be as convenient as possible. But there are some UI problems that hard to solve even on full sized Web browser on PC, that is why I try to experiment with other UI types. Web is not good enough for mobile use because of three things:
Another type of UI can be implemented as standalone hardware client with hardware knobs and push buttons for control (with or without display). On prototype of such client I'm working now. The idea was born to help my friend be able use OpenWebRX SDRs to listen to HamRadio (he can't use traditional Web based UI because of problems with vision ), but now it also looks for me as a pretty convenient way to use OpenWebRX SDRs.
Of course. Please consider this pull-request as bug-report with proposed solution. I will continue testing it on my OpenWebRX setup on RaspberryPi. By the way, is it possible to build arm docker image on x86_64 host with scripts in repo? |
|
No, the docker scripts will only build images for the current architecture. Building docker images for a different architecture would require emulation, which is something that docker does not offer, at least not to my knowledge. |
My thoughts was the same, just asked in case I missed something and it could be some how possible. Thank you! |
| @@ -175,60 +177,67 @@ def read_loop(self): | |||
| def protected_read(num): | |||
| data = self.handler.rfile.read(num) | |||
There was a problem hiding this comment.
all reading should be moved into the loop; just initialize data to an empty array and start with a select. that way the code should be easier to read.
| if self.handler.rfile in read: | ||
| data += self.handler.rfile.read(num - len(data)) | ||
| else: | ||
| if len(data) == 0: |
There was a problem hiding this comment.
The else branch comes into effect if the interrupt pipe has been triggered, so typically there's no need to check the data length or have different types of exceptions here.
| while self.open and len(data) < num: | ||
| (read, _, _) = select.select([self.interruptPipeRecv, self.handler.rfile], [], [], 15) | ||
| if self.handler.rfile in read: | ||
| data += self.handler.rfile.read(num - len(data)) |
There was a problem hiding this comment.
data length should be checked after every read. if the rfile was in the select, but has 0 data available, this typically means the socket has been closed.
| if self.handler.rfile in read: | ||
| available = True | ||
| try: | ||
| header = protected_read(2) | ||
| self.resetPing() |
There was a problem hiding this comment.
i'd suggest moving this to protected_read, too - it should be reset on every bit of data that comes from the client.
| self.open = False | ||
| else: | ||
| logger.warning("unsupported opcode: {0}".format(opcode)) | ||
| except Drained: |
There was a problem hiding this comment.
Drained exception is no longer used, so the class definition (further up) should be removed as well
| if mask: | ||
| masking_key = protected_read(4) | ||
| data = None | ||
| if length > 0: |
There was a problem hiding this comment.
instead of implementing the check here, just pass length=0 to protected_read and let it return early with an empty byte array. i'd say that's more elegant and removes some distracting conditions here.
Fix error when message body cannot be received with non blocking read after receiving header