-
Notifications
You must be signed in to change notification settings - Fork 51
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
Assertion failure #127
Comments
Hi Fabio, I can't reproduce the issue either, however the assertion would fail if:
Note that the Kindly, |
Hi Bo, Still can't reproduce it locally, but we have now more info about last crash in production. The assertion fail after receiving from a WebSocket client frames with unmasked data, with continuation frame inside. I see this client error condition (sending unmasked data) is properly checked in
Kind regards, |
Hi @fabio , Vary nice catch! Yes, adding a return statement there will fix an unexpected code path for the callbacks, but I think it would also be better to fix the callbacks themselves. There's a chance (untested) that there's another way that lead to the same code path, so it's better to fix the callbacks as well... I'll have a look at this later tonight. Cheers, |
Hi @fabio , I committed a possible fix for the issue. However, I want to be sure that the fix performs as expected before releasing a new version. Would you mind testing it, or is that too much to ask for in production? Thanks! |
Sure, I'll give it a try. Cheers, |
The fix has already been in production on some servers for 2 days and will progressively be used by all the others. We may take a few more days to confirm that the fix works well with no side effects. -- fabio |
Hi @fabio , Thank you so much for the update! My main concern is that the code path that allocates a continuation-message buffer wasn't tested when I tested for memory leaks - I don't have a good tool to reproduce a multitude of continuation messages on multiple connections... so that's my main concern. I don't think the assertion will re-appear, and I am sure the buffer should be freed, but that's all theory and isn't tested. If you don't see memory leaks than I think we're good to release the fix. Thanks again! |
Hi @boazsegev, Yes, in fact, I confirm that at the moment I am not detecting memory problems. However I have found another problem in one of the servers with the fix.
Apart form Iodine version and line numbers, it is the same backtrace reported here: #100 (comment). If we could fix that too it would be great. I'm trying to figure out how to reproduce it. Kid Regards, |
I added a NULL test to hopefully solve the #100 issue as well, so I guess I'll release an update and hope for the best. |
System Information
Description
Frequently get assertion failure:
Rack App to Reproduce
Can't reproduce.
Do you have any suggestion about where to try to look for to identify the cause of the failure? I can't find any direct relation with the Ruby app code or some specific user action.
Thanks in advance.
-- fabio
The text was updated successfully, but these errors were encountered: