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 win32 #10784

Merged

Conversation

straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Jun 4, 2021

This patch implements Crystal::System::Socket for Windows using the winsock2 API, as proposed in #10696.

Most IO methods are implemented as overlapped operations based on IO completion ports. Since the event loop implementation is still missing, the fiber swapping mechanism is currently stubbed as a wait for the current operation to complete. In a follow up, this can be replaced by a proper event loop implementation without requiring any changes to the socket implementation. I already have a dev branch with the event loop.

A new IO::Overlapped module is introduced as the counterpart of IO::Evented for overlapped operations. This setup is subject to refactorings as currently discussed in #10766, but this should be fine for the initial implementation.

There are still a couple of minor issues documented in the specs (either as pending_win32 or excluded via flag?(:win32). I'll keep on working on that, but it's not critical for the basic implementation.

Thanks to @andraantariksa, @cfsamson, @kubo, @neatorobito, @yxhuvud et al. for your preliminary work, exploration, ideas and hopefully reviews on this one.

The first two commits are from #10782. I'll rebase the branch once that's merged.

Closes #9544
Closes #10696

Comment on lines +175 to +176
output_buffer.to_unsafe.as(Void*), buffer_size.to_u32!,
address_size.to_u32!, address_size.to_u32!, pointerof(received_bytes), overlapped)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't see the point of these conversions, is there any reason to not create them as u32 from the start?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value is based on sizeof which returns Int32, so there must be a conversion at some point. I suppose it could be done earlier, though.
The main reason for this is that the proc call resembles a lib call. The lib call would handle all this type casting (and to_unsafe conversion), but we need to make it explicit here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. comment was also about buffer_size though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buffer_size is also used in an arithmetic expresssion above, and signed integers is better for this. There needs to be a conversion somewhere, but I prefer to do it closely to the proc call.

src/crystal/system/win32/socket.cr Show resolved Hide resolved
src/lib_c/x86_64-windows-msvc/c/ws2ipdef.cr Outdated Show resolved Hide resolved
src/crystal/system/win32/event_loop_iocp.cr Show resolved Hide resolved
@straight-shoota
Copy link
Member Author

straight-shoota commented Jun 7, 2021

I'm really confused about the CI error:

Run .\std_spec.exe --verbose -e TCPSocket
0.2.4
TCPSocket
  #connect
    using IPv4
      connects to server
      connects to server
Error: Process completed with exit code 1.

(I've isolated the TCPSocket specs because that's consistently where it breaks)

The specs run fine and successfully locally.

What could be causing the process to exit? Without any kind of error message? It's seemingly between specs: the first line connects to server should be printed before, second line after the example runs. I tried disabling the first couple of specs but then the error just happens later in the file.

Copy link
Contributor

@cfsamson cfsamson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Left a few comments, but over all this looks good to me.

src/crystal/system/win32/socket.cr Outdated Show resolved Hide resolved
src/crystal/system/win32/socket.cr Outdated Show resolved Hide resolved
@straight-shoota straight-shoota marked this pull request as draft June 8, 2021 13:18
@straight-shoota
Copy link
Member Author

straight-shoota commented Jun 10, 2021

There is something very strange going on here.

I'm pretty sure this is some kind of codegen error, or anything else outside the scope of this patch. The same code works locally, but fails in CI. The minimal difference between failing and succeeding that I could identify by now is this:

--- c/src/socket/addrinfo.cr
+++ w/src/socket/addrinfo.cr
@@ -66,6 +66,8 @@ class Socket
               if value.is_a?(Socket::ConnectError)
                 raise Socket::ConnectError.from_os_error("Error connecting to '#{domain}:#{service}'", value.os_error)
               else
+                StaticArray(UInt8, 0).new(0_u8)
+
                 raise value
               end
             end

The only relevant changes to the LLVM IR are these:

@@ -155358,8 +155407,7 @@ then98:                                           ; preds = %else95
           to label %invoke_out100 unwind label %rescue

 else99:                                           ; pqreds = %else95
-  %234 = load i32*, i32** %value
-  invoke void @.2A.raise.3C.Socket.3A..3A.Error.2B..3E..3A.NoReturn(i32* %234)
+  %234 = invoke [0 x i8] @.2A.StaticArray.28.UInt8.2C..20.0.29..40.StaticArray.28.T.2C..20.N.29..3A..3A.new.3C.UInt8.3E..3A.StaticArray.28.UInt8.2C..20.0.29.(i32 1435, i8 0)
           to label %invoke_out103 unwind label %rescue

 invoke_out100:                                    ; preds = %then98
@@ -155378,21 +155426,26 @@ invoke_out102:                                    ; preds = %invoke_out101
   unreachable

 invoke_out103:                                    ; preds = %else99
+  %240 = load i32*, i32** %value
+  invoke void @.2A.raise.3C.Socket.3A..3A.Error.2B..3E..3A.NoReturn(i32* %240)
+          to label %invoke_out104 unwind label %rescue
+
+invoke_out104:                                    ; preds = %invoke_out103
   unreachable

I used bin/crystal build --cross-compile --target x86_64-windows-msvc spec/std/socket/tcp_socket_spec.cr --emit llvm-ir --no-debug to generate the IR.
That doesn't really look like much. The only semantic change is inserting the call to StaticArray.new, as expected.

But, with that call missing, the process seems to immediately terminate around this location. I don't think it actually goes into this branch.

If everything else is green, I'd probably leave that weird workaround for this PR and open a new issue to investigate the error.

Copy link
Member

@beta-ziliani beta-ziliani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some superficial questions for now.

src/io/overlapped.cr Show resolved Hide resolved
src/lib_c/x86_64-windows-msvc/c/ws2ipdef.cr Show resolved Hide resolved
src/lib_c/x86_64-windows-msvc/c/winsock2.cr Show resolved Hide resolved
Copy link
Member

@beta-ziliani beta-ziliani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides of the puzzling-looking (to me at least) FIXME, I'd say this is good to go

spec/std/socket/tcp_socket_spec.cr Show resolved Hide resolved
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.

Porting Socket to win32
4 participants