Skip to content
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

Fix multiple TLS records in buffer #2890

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix multiple TLS records in buffer #2890

wants to merge 1 commit into from

Conversation

scaprile
Copy link
Collaborator

@scaprile scaprile commented Sep 9, 2024

Even though we receive IO_ERR, several outstanding TLS records can lie in our buffer. One or two extra calls is not enough to correctly process them and cleanly close.

NOTE: is_io_err becomes unused, will be removed further on if this proves to be the final battle.

#2885

@jcorporation
Copy link

It seems to work more often than before, but not always. Attached a log with a failed download.
log2.txt

@scaprile
Copy link
Collaborator Author

scaprile commented Sep 9, 2024

@jcorporation I can't reproduce this. With this patch I get 20 out of 20 correct downloads, 107586

@jcorporation
Copy link

Tried it again. Most times it works, but sometime it fails.

I freshly cloned latest master and merged the tls branch.

@scaprile
Copy link
Collaborator Author

scaprile commented Sep 9, 2024

I could reproduce it 1 in a 1000 tries...
I'll add some debug helpers and see what I can do. Most likely we'll merge this patch (or equivalent) and try to isolate and fix the low failure rate bug later.

@jcorporation
Copy link

My failure rate is much higher. I tested with a wlan client. I will repeat my tests with lab and report back.

You tested with the url from my last tests?

@scaprile
Copy link
Collaborator Author

scaprile commented Sep 9, 2024

Yes, 10000 iterations 10 failures
Maybe the problem is an incorrect response to a connection loss now ? I'll probably check tomorrow.

@jcorporation
Copy link

jcorporation commented Sep 9, 2024

I tested on a wired device and experienced the same failures regularly. I will check it with a different internet connection.

I tested the same example with mongoose 7.14 and the error is gone. It seems to be a regression introduced in mongoose 7.15.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants