Project

General

Profile

Actions
Copy link

Bug #4099

closed

StreamTransport should not go into FAILED state when remote host gracefully closes the connection

Added by Davide Pesavento over 7 years ago. Updated over 7 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Davide Pesavento
Category:
Faces
Target version:
v0.6
Start date:
Due date:
% Done:

100%

Estimated time:

Description

Seeing

1496093845.757178 INFO: [Transport] [id=260,local=unix:///run/nfd.sock,remote=fd://26] setState UP -> FAILED
1496093845.757266 INFO: [Transport] [id=260,local=unix:///run/nfd.sock,remote=fd://26] setState FAILED -> CLOSED

in the logs every time a local application closes its socket is unexpected and confusing.

According to the following doxygen

enum class TransportState {
  ...
  CLOSING, ///< the transport is requested to be closed
  FAILED, ///< the transport is being closed due to a failure
  ...

FAILED is definitely wrong, because there is no failure, in fact the connection is being closed gracefully by the remote host or application (we get an EOF on the socket). The description of CLOSING is ambiguous (requested by who?), but is more appropriate than FAILED.

Another option is introducing an additional REMOTE_CLOSE state, but I believe this would be overkill.

Yet another option is using CLOSING for on-demand faces, and keep FAILED for persistent faces.

Actions
Copy link
 #1

Updated by Davide Pesavento over 7 years ago

  • Status changed from New to Code review
Actions
Copy link
 #2

Updated by Junxiao Shi over 7 years ago

CLOSING, ///< the transport is requested to be closed

This is the problem: CLOSING requires a request, i.e. a Transport::close call.
You can introduce an ENDING state to differentiate from FAILED.

Actions
Copy link
 #3

Updated by Davide Pesavento over 7 years ago

Junxiao Shi wrote:

CLOSING, ///< the transport is requested to be closed

This is the problem: CLOSING requires a request, i.e. a Transport::close call.

As I said, the description is unclear. A FIN packet from a remote host is in a way a request to close the connection. In any case, nothing relies on the exact semantics of CLOSING vs. FAILED, so this is a pretty arbitrary argument at the moment.

You can introduce an ENDING state to differentiate from FAILED.

Overkill, there's no reason to distinguish.

Actions
Copy link
 #4

Updated by Davide Pesavento over 7 years ago

Another thing to note is that WebsocketTransport sets the state to CLOSING when the remote endpoint closes the connection, so there's already inconsistency between WebsocketTransport and StreamTransport.

Yet another option is using CLOSING for on-demand faces, and keep FAILED for persistent faces.

I didn't get a clear answer on this. Would the above approach be preferable or is it unnecessary complexity?

Actions
Copy link
 #5

Updated by Junxiao Shi over 7 years ago

Another thing to note is that WebsocketTransport sets the state to CLOSING when the remote endpoint closes the connection, so there's already inconsistency between WebsocketTransport and StreamTransport.

As I said on Gerrit: TransportState::CLOSING is currently defined as having received a .close() call. If you intend to extend this state to also indicate having received an EOF, the Doxygen of that state needs be be updated.
I'm OK with using CLOSING to represent a remote FIN, but TransportState::CLOSING documentation shall be updated accordingly.

Yet another option is using CLOSING for on-demand faces, and keep FAILED for persistent faces.

If CLOSING definition is extended, it should apply to both on-demand and persistent faces.

Actions
Copy link
 #6

Updated by Davide Pesavento over 7 years ago

  • Status changed from Code review to Closed
  • % Done changed from 0 to 100
Actions
Copy link

Also available in: Atom PDF