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

Make close synchronous #15

Closed
wants to merge 1 commit into from

Conversation

puddly
Copy link
Collaborator

@puddly puddly commented Jul 14, 2024

This PR makes close truly synchronous by:

  1. Not calling ser.flush() when closing the serial port. This is a breaking change but I don't think it was correct to wait for flush to be called anyways.
  2. Calling ser.close() within the event loop, not in an executor thread. I believe this should be safe given the implementation. All of the file descriptors are opened in non-blocking mode.

For background on point 2, see pyserial#82. I believe that this codepath (TCP socket:// URIs) is now properly special cased and handled within pyserial-asyncio-fast. At this point, pyserial should be used only for serial ports.

Fixes #7.

@cottsay
Copy link

cottsay commented Jul 14, 2024

What does this mean for SerialTransport.abort()?

Isn't the intended difference between close() and abort() simply that abort() doesn't flush but close() does?

@puddly
Copy link
Collaborator Author

puddly commented Jul 14, 2024

Good point.

The semantics for WriteTransport.abort make it sound like it's harsher than close:

Close the transport immediately, without waiting for pending operations to complete. Buffered data will be lost. No more data will be received. The protocol’s protocol.connection_lost() method will eventually be called with None as its argument.

WriteTransport.close waits for data to be flushed.

It may just be me but it seems like a common assumption for transports to be closed when you call close. It seems like the correct behavior is to wait for the connection_lost event.

@puddly
Copy link
Collaborator Author

puddly commented Jul 14, 2024

I think #16 is the right fix. This approach only works for POSIX and maintains the same bug in event ordering.

@puddly puddly closed this Jul 14, 2024
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.

Task was destroyed but it is pending!
2 participants