Traffic Server
  1. Traffic Server
  2. TS-320

Do some cleanup on Connection::fast_connect and Connection::bind_connect

    Details

      Description

      Action Plan:

      Unify bind_connect, fast_connect.

      1. Split out the binding of the target address in to a new method
      Connection::bind_target(target_ip, target_port);

      This simply copies to the target IP address and port to Connection::sa. As far as I can tell this is the only thing the calls to bind_connection from IPC.cc do. We can then change those calls to direct calls to bind_target. Then we should be able to eliminate all the bool flags on bind_connect which aren't used elsewhere.

      2. Remove the my_ip argument from bind_connect. Use spoof_ip in NetVCOptions instead. At this point the signatures for fast_connect and bind_connect are now identical.

      3. Add support for the IP_TRANSPARENT sock opt provided by TPROXY in to NetVCOptions, either as a separate flag or (better) merged in to NetVCOptions::sockopt_flags. This would mean that spoof_ip is treated as a local address (i.e., an address already assigned to an interface on the local system) if not set, or as an arbitrary address requiring TPROXY support if set. If we conditionally define the mask as IP_TRANSPARENT or 0 depending on TPROXY availability we should be able to limit conditional compilation to that location (as the check will always fail if IP_TRANSPARENT is not available).

      4. Move SO_REUSEADDR to NetVCOptions::sockopt_flags. This is set in bind_connect but not fast_connect.

      5. Put an TCP vs. UDP flag in NetVCOptions. This would follow from the code, but as far as I can tell this is never used. It is passed in from the ICP logic but, because that also sets the "no bind" flag, it doesn't get invoked. It may be that bind_connect should just be implicitly TCP.

      6. Verify that NetVCOptions default constructed values are such that bind_connect with a default constructed NetVCOptions does the same thing that fast_connect does.

      7. Add an option (in the signature or in NetVCOptions) to handle the epoll support. AFAICT this is done in all cases where either fast_connect or bind_connect is called other than IPC (which we have already removed) so we could hard wire it. Comments in the code indicated this is already considered a problem.

      After all of this, we can replace all invocations of fast_connect with bind_connect with no change in functionality.

      Note: This is in preparation for working on TS-291.

      1. ts-320-patch.txt
        44 kB
        Alan M. Carroll

        Issue Links

          Activity

          Hide
          ASF subversion and git services added a comment -

          Commit 05f7bfb57517e60a364a62dd171e5cc65824aa4a in branch refs/heads/master from Gota Adachi
          [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=05f7bfb ]

          TS-32: minimum FIX to let ICP work

          Fixed an error on access to class member that was not initialized and
          restored former implementation based on a change in TS-320.

          And commented out the forced termination code in
          UDPReadContinuation::readPollEvent().
          Please tell me if you know what seems to be the problem in this
          implementation.

          Signed-off-by: Zhao Yongming <ming.zym@gmail.com>

          Show
          ASF subversion and git services added a comment - Commit 05f7bfb57517e60a364a62dd171e5cc65824aa4a in branch refs/heads/master from Gota Adachi [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=05f7bfb ] TS-32 : minimum FIX to let ICP work Fixed an error on access to class member that was not initialized and restored former implementation based on a change in TS-320 . And commented out the forced termination code in UDPReadContinuation::readPollEvent(). Please tell me if you know what seems to be the problem in this implementation. Signed-off-by: Zhao Yongming <ming.zym@gmail.com>
          Hide
          Alan M. Carroll added a comment -

          Comitted by zwoop.

          Show
          Alan M. Carroll added a comment - Comitted by zwoop.
          Hide
          Alan M. Carroll added a comment -

          Updated patch for version 955332.

          Show
          Alan M. Carroll added a comment - Updated patch for version 955332.
          Hide
          Alan M. Carroll added a comment -

          I am still waiting on the permissions. I am also working on an updated patch because there were conflicting changes from the TS-18 merge.

          I will also remove the #if 0 code. That's just an artifact of not being able to check in intermediate states.

          The IP_TRANSPARENT issue is not currently a problem, as none of the other code actually sets that option. At this point I will fix that in the transparency patch (TS-291) by tweaking autoconfig to check for the header file. That seems more robust.

          Let me think about the cleaner. I am not satisfied that it is sufficiently general purpose to be moved to a more general header file. I think that should be left for another distinct patch.

          Actually, I added the is_connect and is_bound to correspond to the checks on the socket file descriptor that existed in the previous code. I think it's worth keeping that in to detect potential issues if these methods are used elsewhere (which, IMHO, should be a goal, because there's still a lot of duplicated code for connecting in various places).

          Show
          Alan M. Carroll added a comment - I am still waiting on the permissions. I am also working on an updated patch because there were conflicting changes from the TS-18 merge. I will also remove the #if 0 code. That's just an artifact of not being able to check in intermediate states. The IP_TRANSPARENT issue is not currently a problem, as none of the other code actually sets that option. At this point I will fix that in the transparency patch ( TS-291 ) by tweaking autoconfig to check for the header file. That seems more robust. Let me think about the cleaner. I am not satisfied that it is sufficiently general purpose to be moved to a more general header file. I think that should be left for another distinct patch. Actually, I added the is_connect and is_bound to correspond to the checks on the socket file descriptor that existed in the previous code. I think it's worth keeping that in to detect potential issues if these methods are used elsewhere (which, IMHO, should be a goal, because there's still a lot of duplicated code for connecting in various places).
          Hide
          John Plevyak added a comment -

          Reviewed, looks good, here are a few comments

          • This patch fixes a potential problem with EDGE_TRIGGERED on FreeBSD/Solaris when an error occurs
            whereby the events might not be cleared before the close().
          • FreeBSD and Solaris queue the kqueue/poll() events in user space and only flush them
            at the poll, so it is best not to start() then close() as the event will still be in the queue and
            could be applied to a different connection with the same socket #
          • I would remove #if 0 in connectUp and the dead code above it
          • Do we want to try the safe_setsockopt in Connection::open if IP_TRANSPARENT
            is not defined ? Is it possible for there to be a conflict on # 19 ?
          • Should move the cleaner from UnixConnection.cc into a more general place
            (libinktomi++ ?)
          • is_connected and is_bound in Connection look like they are old debugging cruft,
            we might want to get rid of them.

          None of this needs to be fixed before checkin since it is existing.
          I would encourage checkin sooner rather than later because of the EDGE_TRIGGER issue.

          I assume that now you have the perms you want to checkin? Otherwise, tell me and I can.

          Show
          John Plevyak added a comment - Reviewed, looks good, here are a few comments This patch fixes a potential problem with EDGE_TRIGGERED on FreeBSD/Solaris when an error occurs whereby the events might not be cleared before the close(). FreeBSD and Solaris queue the kqueue/poll() events in user space and only flush them at the poll, so it is best not to start() then close() as the event will still be in the queue and could be applied to a different connection with the same socket # I would remove #if 0 in connectUp and the dead code above it Do we want to try the safe_setsockopt in Connection::open if IP_TRANSPARENT is not defined ? Is it possible for there to be a conflict on # 19 ? Should move the cleaner from UnixConnection.cc into a more general place (libinktomi++ ?) is_connected and is_bound in Connection look like they are old debugging cruft, we might want to get rid of them. None of this needs to be fixed before checkin since it is existing. I would encourage checkin sooner rather than later because of the EDGE_TRIGGER issue. I assume that now you have the perms you want to checkin? Otherwise, tell me and I can.
          Hide
          Alan M. Carroll added a comment -

          This patch cleans up Connection::fast_connect, bind_connect, and connect in to two cooperating methods. All arguments beyond target IP address and port are rolled in to the NetVCOption struct. Control of the default values is isolated in the NetVCOptions::reset method.

          This doesn't fully clean up the socket options but it makes it easy to add or control them by tweaking just NetVCOptions and Connection::open.

          Show
          Alan M. Carroll added a comment - This patch cleans up Connection::fast_connect, bind_connect, and connect in to two cooperating methods. All arguments beyond target IP address and port are rolled in to the NetVCOption struct. Control of the default values is isolated in the NetVCOptions::reset method. This doesn't fully clean up the socket options but it makes it easy to add or control them by tweaking just NetVCOptions and Connection::open.
          Hide
          Alan M. Carroll added a comment -

          As usual, the plan did not survive contact with the codebase.

          In the end, the low level connect calls were invoked only from one place, UnixNetVConnection::connectUp. Therefore the result was tailored to the needs of that call site. Connection::connect, Connection::fast_connect, and Connection::bind_connect were replaced by two new methods.

          open: Create the socket and set all options that are set before connection.
          connect: Connect the socket to the target. Set options that must be set after connection.

          These are only somewhat similar to the original methods. All arguments other than the remote IP address and port have been moved in to the NetVCOptions structure. The default values for this are controlled solely by the NetVCOptions::reset method. All can be overridden, none of them are hardwired or "re-defaulted" in the Connection methods.

          For the ICP code, that at last went as orginal intended. It continued to be the case that as far as I can tell the Connection object is used only to store a sockaddr_in. It's not completely clear because the ICP code wasn't updated when the socket argument was added to Connection::bind_connect. Therefore I made a method to set the internal sockaddr_in and had that called from the ICP code instead of Connection::bind_connect.

          Show
          Alan M. Carroll added a comment - As usual, the plan did not survive contact with the codebase. In the end, the low level connect calls were invoked only from one place, UnixNetVConnection::connectUp. Therefore the result was tailored to the needs of that call site. Connection::connect, Connection::fast_connect, and Connection::bind_connect were replaced by two new methods. open: Create the socket and set all options that are set before connection. connect: Connect the socket to the target. Set options that must be set after connection. These are only somewhat similar to the original methods. All arguments other than the remote IP address and port have been moved in to the NetVCOptions structure. The default values for this are controlled solely by the NetVCOptions::reset method. All can be overridden, none of them are hardwired or "re-defaulted" in the Connection methods. For the ICP code, that at last went as orginal intended. It continued to be the case that as far as I can tell the Connection object is used only to store a sockaddr_in. It's not completely clear because the ICP code wasn't updated when the socket argument was added to Connection::bind_connect. Therefore I made a method to set the internal sockaddr_in and had that called from the ICP code instead of Connection::bind_connect.
          Hide
          Alan M. Carroll added a comment -

          This may cover TS-247 but I won't know until most of the work is done.

          Show
          Alan M. Carroll added a comment - This may cover TS-247 but I won't know until most of the work is done.
          Hide
          Alan M. Carroll added a comment -

          This restructuring should fix TS-300 as well.

          Show
          Alan M. Carroll added a comment - This restructuring should fix TS-300 as well.
          Hide
          Alan M. Carroll added a comment -

          This incorporates the parts of TS-382 that involve outbound connections. It does not affect any of the inbound / accept issues.

          Show
          Alan M. Carroll added a comment - This incorporates the parts of TS-382 that involve outbound connections. It does not affect any of the inbound / accept issues.

            People

            • Assignee:
              Alan M. Carroll
              Reporter:
              Alan M. Carroll
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 168h
                168h
                Remaining:
                Remaining Estimate - 168h
                168h
                Logged:
                Time Spent - Not Specified
                Not Specified

                  Development