-
Notifications
You must be signed in to change notification settings - Fork 7
test: make sure that underlying connection is closed #49
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
Conversation
LeonidVas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HI! Thank you for the patch.
I suggest using in title something like:" test: add test for ...".
Add Closes #31
AFAIU, you save the descriptor, close it and expect the same descriptor will be used for a connection. This is a good hypothesis, but please add a comment to your hypothesis. Also, I'm expecting some links to prove your hypothesis (if possible).
Is this only true for linux or other OS too? Will such test work on MacOS, WSL, WSL2? Maybe run this test on Linux only?
cedb080 to
4563da5
Compare
I couldn't find any directly obvious proof, but let me try. Basing on the statement, that file descriptors are recycled in ascending order. It means, that if we call |
4563da5 to
cababcf
Compare
The kernel code: https://github.com/torvalds/linux/blob/585e5b17b92dead8a3aca4e3c9876fbca5f7e0ba/fs/file.c#L503-L510 The simplified description of this code: https://www.win.tue.nl/~aeb/linux/vfs/trail-2.html Another description: |
cababcf to
a0fcdf4
Compare
Alexander found confirmation. Please add this as a comment to your code with describing how the test works. |
a0fcdf4 to
546c352
Compare
Done. |
546c352 to
48753a3
Compare
LeonidVas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Totktonada
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Closes #31