-
-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
protocol error from tungstenite on send is bug in ws_stream_tungstenite #7
Comments
Thanks for reporting. I should have time to look into it tomorrow. ws_stream_tungstenite needs an update anyway, but I have to debug a stack overflow coming from tungstenite with the latest version. |
Do I get it right that it's rumqttc that is invoking ws_stream_tungstenite? Could you point me to the code that is invoking rumqttc? Also, does this only happen with the new version (v0.10.0) of rumqttc? |
Yes, it's with v0.10.0, the MQTT code is here https://github.com/iotaledger/iota.rs/blob/dev/iota-client/src/node/mqtt.rs and for the python binding here https://github.com/iotaledger/iota.rs/blob/dev/bindings/python/native/src/client/mqtt.rs |
Thanks for clarifying.
That's unfortunate. Ideally we find a way to reproduce consistently. Is there a command which I can run on the repo offline to reproduce the failing test? |
Agree, unfortunately I couldn't reproduce it yet |
Thanks for trying. We can leave it open for now. I will try to look into it anyways, because this should never happen. Maybe tungstenite have changed something since I wrote it. |
Im about to roll out a new version with up to date tungstenite. I have not been able to find what provoked this bug. From what I can see in the tungstenite code, the only way to trigger a protocol error on send is by sending after closing the websocket. AFAICT we never do that. So a bug is hiding somewhere... @tekjar if you have any idea how this can happen? In any case I have added a bit more information on the error variant in case this happens again. If someone can find a way to reproduce it would really help. If not at least a backtrace would already help. I'll leave this open until we find it. |
Sadly
Thus, it's a breaking change and the minor version number should be incremented instead of the patch version number (i.e. |
@lmy441900 Oh. Sorry my bad. Updating async-tungstenite is a breaking change. I will fix that. |
0.7.0 has been released and 0.6.2 has been yanked. |
Hi, can you still reproduce this with the latest version? If not I would like to close the issue. |
rumqttc isn't updated to this version yet, but I also never saw this occur again, so I guess it's fine to close |
The same error has just happened to me while using rumqttc 0.22.0. I've had this version deployed to multiple IoT devices and it has happened on just one of them after a couple of days. I don't really have any more details I could provide at this moment, but I'll try to monitor it more closely. |
ah, damn Im sorry to hear that. Can you update to the latest version? The code throwing this error has changed. |
rumqttc 0.22.0` uses ws_stream_tungstenite "0.10", in rumqttc 0.23.0 ( latest ) we are using "0.11" ( latest ), so updating rumqttc might work as said by @najamelan . Also there aren't any major breaking changes, so update should be smooth. |
Sorry, I have mistyped and I'm already using rumqttc 0.23.0. It's however a fork that adds support for |
Do you have the exact line number that panicked? |
OOH that PR! didn't realize it was yours haha I did work on that PR after our discussion ended, even in last week, with / without will get back with more details :) |
btw, I didn't use the exact fork ( had some of my changes as mentioned in comments of PR & here ), but I didn't face such panic. |
This is all the information I've found in logs:
|
Thanks I'll have a look at that. |
It seems this isn't as random as I thought it'd be. It eventually happened on all 6 RPis I've deployed this code on - it always took a couple of weeks to emerge though. Unfortunatelly, I still don't have any more clues I could provide :/ |
ah, sorry to hear. I think there is 2 things we can do, given this should mean that ws_stream_tungstenite violates the WS protocol:
I just need to find time to dedicate to it as life is a bit hectic ATM. Feel free to review the code, and see the tungstenite code for when they throw this error. |
Just got this error
in a test from our workflow https://github.com/iotaledger/iota.rs/runs/4060223475?check_suite_focus=true
The text was updated successfully, but these errors were encountered: