Skip to content

Conversation

@JosiahWI
Copy link
Contributor

@JosiahWI JosiahWI commented Jun 26, 2024

UnixSocket Class

The new class is a thin wrapper for a Unix socket file descriptor. It does
not manage the resource itself, although this would be a good idea for a
future improvement.

Removed Functions

Some functions are not applicable to sockets and do not belong in a utility
for managing sockets. The functions open, pwrite, lseek and fsync were not
transferred to the new class. The function socket() was replaced by a
UnixSocket constructor.

API Changes

The file descriptor no longer needs to be passed because it is remembered
by the UnixSocket object. Some parameters that were unused are removed
from the new interface. There is no longer a socket() function; instead
a UnixSocket constructor with the same arguments is provided.

Implementation Changes

Other than small cleanups like changing uint64_t to std::uint64_t and
constifying parameters, all implementations are directly copied from
the old SocketManager ones except for client_fastopen_supported. It
was refactored to improve naming and make the use of the bitmask obvious.

Performance Considerations

Since the class is a thin wrapper, there should be no performance overhead
from the use of a class. All methods that were declared inline in a header
for UnixSocket are also declared inline in the UnixSocket.h header. There
could be a small increase in compile times because there is no separate
P_UnixSocket.h header, but UnixSocket.h is private to inkevent so far, so
the compilation time difference should be negligible.

@JosiahWI JosiahWI added this to the 10.1.0 milestone Jun 26, 2024
@JosiahWI JosiahWI self-assigned this Jun 26, 2024
@JosiahWI JosiahWI force-pushed the refactor/unix-socket branch from 786ba06 to 6d2f481 Compare June 26, 2024 13:50
JosiahWI added 2 commits June 26, 2024 08:58
UnixSocket Class

  The new class is a thin wrapper for a Unix socket file descriptor. It does
  not manage the resource itself, although this would be a good idea for a
  future improvement.

Removed Functions

  Some functions are not applicable to sockets and do not belong in a utility
  for managing sockets. The functions open, pwrite, lseek and fsync were not
  transferred to the new class. The function socket() was replaced by a
  UnixSocket constructor.

API Changes

  The file descriptor no longer needs to be passed because it is remembered
  by the UnixSocket object. Some parameters that were unused are removed
  from the new interface. There is no longer a socket() function; instead
  a UnixSocket constructor with the same arguments is provided.

Implementation Changes

  Other than small cleanups like changing uint64_t to std::uint64_t and
  constifying arguments, all implementations are directly copied from
  the old SocketManager ones except for client_fastopen_supported. It
  was refactored to improve naming and make the use of the bitmask obvious.

Performance Considerations

  Since the class is a thin wrapper, there should be no performance overhead
  from the use of a class. All methods that were declared inline in a header
  for UnixSocket are also declared inline in the UnixSocket.h header. There
  could be a small increase in compile times because there is no separate
  P_UnixSocket.h header, but UnixSocket.h is private to inkevent so far, so
  the compilation time difference should be negligible.
@JosiahWI JosiahWI force-pushed the refactor/unix-socket branch from 6d2f481 to 74d925c Compare June 26, 2024 13:58
@cmcfarlen
Copy link
Contributor

imo we should NOT use TS_INLINE for new code. Just use inline where appropriate.

@maskit
Copy link
Member

maskit commented Jun 26, 2024

What's the responsibility of SocketManager? Do we still need it?

@JosiahWI
Copy link
Contributor Author

JosiahWI commented Jun 26, 2024

@maskit Nope, although we have to find a home for the open function before we get rid of it - it's used only in the cache Store so maybe it could go there. I left it in as a compatability layer to protect the rest of ATS from the changes; I will follow this up with (a) separate PR(s) to remove SocketManager. We will have to update inknet, inkdns, inkproxy, and inkhostdb for the new API; I think those changes can all go together. Since this PR is already a sizable chunk for review, I didn't want to go touching every other subsystem at the same time.

@maskit
Copy link
Member

maskit commented Jun 26, 2024

An option for open function is to have a constructor that receives the three parameters instead of an fd. But because we don't use exceptions, we'd need a function to check whether UnixSocket has a valid fd.

@JosiahWI
Copy link
Contributor Author

The open function is used to open a file by pathname. The socket function is used to create a socket. I don't think the open function even makes sense to have here - someone tried to make SocketManager handle both sockets and regular file descriptors at the same time, I think, and that's probably why stuff like lseek and fsync (which aren't used in the codebase) are in SocketManager. You can't seek with a socket nor can you flush it like you would a file to disk.

@maskit
Copy link
Member

maskit commented Jun 26, 2024

The open function is used to open a file by pathname. The socket function is used to create a socket.

You are right. I somehow mixed up the two, and I meant socket() on the previous comment. Functions for files need their own home.

We decided to leave transient_error where it is because it is specific to
the UnixSocket implementation and will not likely be useful anywhere else.
@JosiahWI JosiahWI requested a review from maskit June 27, 2024 02:09
 * Add a method to check whether socket creation succeeded.
@JosiahWI
Copy link
Contributor Author

JosiahWI commented Jun 27, 2024

I have added a function has_socket to check whether the constructor with three arguments was able to get the socket.

@JosiahWI JosiahWI merged commit e5f02fc into apache:master Jun 27, 2024
@JosiahWI JosiahWI deleted the refactor/unix-socket branch June 27, 2024 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants