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

Implement Socket for Windows (WIP) #9544

Closed

Conversation

andraantariksa
Copy link

This is mostly a work in progress.

@straight-shoota straight-shoota added platform:windows Windows support based on the MSVC toolchain / Win32 API pr:wip topic:stdlib:networking labels Jun 24, 2020
@straight-shoota
Copy link
Member

This is really great! Thanks for getting this started.

Unfortunately, I expect sockets won't really work until the event loop is ported to win32 (#6957).

@andraantariksa
Copy link
Author

@straight-shoota Why the "Windows CI" gives me crystal.obj : error LNK2019: unresolved external symbol WSAStartup referenced in function __crystal_main error in "Link Crystal executable" steps, eventhough I have link it using @[Link("WS2_32")]? I think it only happen for WSAStartup function, the other fuction defined in WS2_32 works fine though.

Code: https://github.com/crystal-lang/crystal/pull/9544/files#diff-f2f9112f7cb72f9c35989628ea179284R43

@kubo
Copy link
Contributor

kubo commented Jul 5, 2020

@andraantariksa

Why the "Windows CI" gives me crystal.obj : error LNK2019: unresolved external symbol WSAStartup referenced in function __crystal_main error in "Link Crystal executable" steps, eventhough I have link it using @[Link("WS2_32")]?

Add ws2_32.lib to https://github.com/crystal-lang/crystal/blob/0.35.1/.github/workflows/win.yml#L167

@andraantariksa
Copy link
Author

Thank you @kubo , it solve the problem. Can you explain why the Windows CI passed when WSAStartup is not defined, although I have define bind which also require Ws2_32?

@kubo
Copy link
Contributor

kubo commented Jul 6, 2020

Can you explain why the Windows CI passed when WSAStartup is not defined, although I have define bind which also require Ws2_32?

Just defining bind doesn't require ws2_32. Using bind requires ws2_32. The crystal compiler itself doesn't use bind but use WSAStartup here.

@kubo
Copy link
Contributor

kubo commented Jul 6, 2020

I have some opinions about this PR.

  • I think that this PR will not be merged until the event loop is ported to win32. (Implement Socket for Windows (WIP) #9544 (comment))

  • Too many duplicated and slightly modified code

    For example in src/socket/tcp_socket.cr:

      {% if flag?(:win32) %}
        def initialize(*, socket : LibC::SOCKET, family : Family = Family::INET)
          super socket, family, Type::STREAM, Protocol::TCP
        end
      {% else %}
        def initialize(*, fd : Int32, family : Family = Family::INET)
          super fd, family, Type::STREAM, Protocol::TCP
        end
      {% end %}

    The difference is only the first named argument. I would add Socket::RawFd and use it as follows in order not to duplicate code.
    In src/socket.cr:

    class Socket < IO
    
      ...
    
      {% if flag?(:win32) %}
        alias RawFd = LibC::SOCKET
        INVALID_RAW_FD = LibC::SOCKET.new(-1)
      {% else %}
        alias RawFd = Int32
        INVALID_RAW_FD = -1
      {% end %}
    
      ...
    
      @volatile_fd : Atomic(RawFd)
    
      def fd : RawFd
        @volatile_fd.get
      end

    In src/socket/tcp_socket.cr:

      def initialize(*, fd : Socket::RawFd, family : Family = Family::INET)
        super fd, family, Type::STREAM, Protocol::TCP
      end
  • When a socket function fails, WSAGetLastError() must be used instead of errno.

    In the document about bind:

    a specific error code can be retrieved by calling WSAGetLastError.

    EDITED: The following code doesn't work...
    I would add SocketErrno and Socket::Error.from_socket_errno() and use SocketErrno.value, SocketErrno::ECONNREFUSED and from_socket_errno() in place of Errno.value, Errno::ECONNREFUSED and from_errno().

      {% if flag?(:win32) %}
        class SocketErrno < WinError
          def self.value : self
            WinError.new LibC.WSAGetLastError
          end
          ECONNREFUSED = WinError::WSAECONNREFUSED
          EADDRINUSE = WinError::WSAEADDRINUSE
             ...
        end
      {% else %}
        class SocketErrno < ::Errno
        end
      {% end %}
      def self.from_socket_errno(message : String? = nil, errno : SocketErrno = SocketErrno.value, **opts)
        {% if flag?(:win32) %}
          self.from_winerror(message, errno, **opts)
        {% else %}
          self.from_errno(message, errno, **opts)
        {% end %}
      end

    Well, it may be better to add Socket::Errno instead of SocketErrno. Using Socket::Errno will make the modified code size smaller compared with using SocketError. However I'm afraid of confusion of ::Errno and Socket::Errno in src/socket.cr and src/socket/*.cr.

  • src/errno.cr should be reverted. Constants starting with WSA are not errno. They are Windows error codes.

  • src/lib_c/x86_64-windows-msvc/c/errno.cr should be reverted. Constants starting with WSA in the file are defined in src/winerror.cr. Newly added errno such as EADDRINUSE(100) are declared in Visual C++ errno.h just because POSIX defines them. Windows socket API uses WSAEADDRINUSE(10048) in place of EADDRINUSE in Unix programs.

I've not tested above code. There may be some flaws.
I'm not a maintainer. Maintainers may have other opinions.

@andraantariksa
Copy link
Author

Thank you for your opinion. I have been messed up with the code, I will try to fix it 👍

@kubo
Copy link
Contributor

kubo commented Jul 6, 2020

SocketErrno in #9544 (comment) doesn't work.
I'm an experienced C programmer but a newbie about crystal...

@straight-shoota
Copy link
Member

@kubo's remarks are value, but there's probably no need to put much work into polishing this PR right now. I'd even suggest to convert it to a draft. There's probably not much sense to merge this until the event loop is ported.
It's still great to have sockets prepared for win32, but we have to go step by step. And there's no point in premature polishing work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform:windows Windows support based on the MSVC toolchain / Win32 API topic:stdlib:networking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants