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.

Sign up for GitHub

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

Jump to bottom

core: Correctly close sockets #2357

Merged
merged 13 commits into from
Jul 25, 2024
Merged

core: Correctly close sockets #2357

merged 13 commits into from
Jul 25, 2024

Conversation

fibonacci-matrix
Copy link
Contributor

Closes: #2353

@fibonacci-matrix fibonacci-matrix marked this pull request as draft July 23, 2024 13:30
julianoes
julianoes previously approved these changes Jul 24, 2024
View reviewed changes
Copy link
Collaborator

@julianoes julianoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it, thank you! Keep them coming 😄

@julianoes
Copy link
Collaborator

Windows CI isn't happy somehow: https://github.com/mavlink/MAVSDK/actions/runs/10061001774/job/27817265595?pr=2357#step:7:55

@fibonacci-matrix @julianoes
Update src/mavsdk/core/socket_holder.h
1c106d3 
Co-authored-by: Julian Oes <julian@oes.ch>
@fibonacci-matrix
Copy link
Contributor Author

It turns out that on 64-bit Windows the socket handle is also 64-bit, unlike POSIX systems where it is always int. I will work on it.

For more details see https://stackoverflow.com/questions/1953639/is-it-safe-to-cast-socket-to-int-under-win64

@fibonacci-matrix
Copy link
Contributor Author

@julianoes , @JonasVautherin I don't have much experience with Windows, could you please check if these lines are necessary in the newly created .cpp file?

#ifdef WINDOWS
#ifndef MINGW
#pragma comment(lib, "Ws2_32.lib") // Without this, Ws2_32.lib is not included in static library.
#endif
#else

@julianoes
Copy link
Collaborator

@fibonacci-matrix I don't know either. If someone that knows Windows complains, we can fix it.

@julianoes julianoes marked this pull request as ready for review July 24, 2024 20:00
@julianoes
Copy link
Collaborator

Should be good to be merged, right @fibonacci-matrix?

@fibonacci-matrix
Copy link
Contributor Author

Should be good to be merged, right @fibonacci-matrix?

Yes of course, @julianoes

@fibonacci-matrix
Remove default move constructor
194411b 
It is currently not in use, and the default implementation is not suitable because it does not change _fd to INVALID for the object being copied from
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jul 25, 2024

Quality Gate Passed Quality Gate passed

Issues
8 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@julianoes julianoes merged commit da13d0a into mavlink:main Jul 25, 2024
31 checks passed
julianoes added a commit that referenced this pull request Jul 25, 2024
@fibonacci-matrix @JonasVautherin @julianoes
core: Correctly close sockets (#2357)
73b42d3 
* core: Correctly close sockets

* Update socket_holder.cpp - Fix newline style

* Update socket_holder.cpp

* Update src/mavsdk/core/tcp_client_connection.cpp

Co-authored-by: Jonas Vautherin <accounts@jonas.vautherin.ch>

* Update socket_holder.cpp - fix code style

* Update tcp_client_connection.cpp - fix code style

* Update tcp_server_connection.cpp - fix code style

* Update socket_holder.h - fix code style

* Update src/mavsdk/core/socket_holder.h

Co-authored-by: Julian Oes <julian@oes.ch>

* Update socket_holder.h - use 64 bit descriptor type on Win64

* Update socket_holder.cpp - use 64 bit descriptor type on Win64

* Update socket_holder.cpp - minor improving of if logic

* Remove default move constructor

It is currently not in use, and the default implementation is not suitable because it does not change _fd to INVALID for the object being copied from

---------

Co-authored-by: Jonas Vautherin <accounts@jonas.vautherin.ch>
Co-authored-by: Julian Oes <julian@oes.ch>
@julianoes julianoes mentioned this pull request Jul 25, 2024
Merged
@fibonacci-matrix fibonacci-matrix deleted the socket-close branch July 25, 2024 07:43
ddatsko pushed a commit to ddatsko/MAVSDK that referenced this pull request Aug 6, 2024
@fibonacci-matrix @JonasVautherin
@julianoes @ddatsko
core: Correctly close sockets (mavlink#2357)
f63eade 
* core: Correctly close sockets

* Update socket_holder.cpp - Fix newline style

* Update socket_holder.cpp

* Update src/mavsdk/core/tcp_client_connection.cpp

Co-authored-by: Jonas Vautherin <accounts@jonas.vautherin.ch>

* Update socket_holder.cpp - fix code style

* Update tcp_client_connection.cpp - fix code style

* Update tcp_server_connection.cpp - fix code style

* Update socket_holder.h - fix code style

* Update src/mavsdk/core/socket_holder.h

Co-authored-by: Julian Oes <julian@oes.ch>

* Update socket_holder.h - use 64 bit descriptor type on Win64

* Update socket_holder.cpp - use 64 bit descriptor type on Win64

* Update socket_holder.cpp - minor improving of if logic

* Remove default move constructor

It is currently not in use, and the default implementation is not suitable because it does not change _fd to INVALID for the object being copied from

---------

Co-authored-by: Jonas Vautherin <accounts@jonas.vautherin.ch>
Co-authored-by: Julian Oes <julian@oes.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JonasVautherin JonasVautherin JonasVautherin left review comments

@julianoes julianoes julianoes left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TCP connection from MAVSDK breaks app after 16 minutes of idle time due to reconnection
3 participants
@fibonacci-matrix @julianoes @JonasVautherin