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 possible AttributeError and OSError on calling TCPTransport.close() #438

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

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).

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

Successfully merging this pull request may close these issues.

1 participant