Skip to content

Fix possible AttributeError and OSError on calling TCPTransport.close() #438

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

Merged
merged 1 commit into from
May 16, 2025

Conversation

Thom747
Copy link
Contributor

@Thom747 Thom747 commented Nov 5, 2024

Pull request

Choose Correct

  • bug
  • feature

Describe the pull request

This PR fixes two bugs:

  • It is possible for socket.shutdown or socket.close to throw OSErrors if the underlying file descriptor is no longer valid. A typical scenario where this could occur is when the remote party of a TCP connection closes the connection and the kernel cleans up the descriptor before the local party can call TCPTransport.close. This error is now caught, as the close effectively did what it intended to do: make sure the socket is closed.
  • TCPTransport.close may be called multiple times, typically when both sides of a TCP connection try to close at the same time. In this case, both _base_receive and some user code call close in any order, resulting in an AttributeError, as _sock would be set to None by the first call. This error is now guarded against by locking and checking that _sock is not None yet.

To Reproduce

  • To reproduce the OSError: close a TCP connection created by a TCPTransport a bunch of times from the remote node, then call close on the TCPTransport. Not sure how deterministic this is on other systems, but in our system this was almost guaranteed to cause this problem.
  • The reproduce the AttributeError: simply call close multiple times. For a more realistic scenario, have the remote party and the local party attempt to close the TCPTransport at roughly the same times.

Expected behavior

These errors are not propagated to the user.

Desktop (please complete the following information):

  • OS: Docker image debian:bookworm-slim with eRPC installed.
  • eRPC Version: v1.13.0

Steps you didn't forgot to do

  • I checked if other PR isn't solving this issue.
  • I read Contribution details and did appropriate actions.
  • PR code is tested.
  • PR code is formatted.
  • Allow edits from maintainers pull request option is set (recommended).

@Thom747
Copy link
Contributor Author

Thom747 commented Mar 27, 2025

@Hadatko or @MichalPrincNXP This PR has not been handled for almost half a year now. Could one of you pick this up?

@MichalPrincNXP MichalPrincNXP self-assigned this May 16, 2025
@MichalPrincNXP MichalPrincNXP changed the base branch from develop to main May 16, 2025 08:50
@MichalPrincNXP MichalPrincNXP force-pushed the fix-tcptransport-errors branch from 5a65350 to 6f15db1 Compare May 16, 2025 08:50
@MichalPrincNXP MichalPrincNXP merged commit 21b43ec into EmbeddedRPC:main May 16, 2025
2 checks passed
@MichalPrincNXP
Copy link
Member

Thank you for the effort and sorry for the delayed response.

@Thom747 Thom747 deleted the fix-tcptransport-errors branch June 3, 2025 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants