Skip to content

Exposes socket connection error into pony level#1520

Closed
zevlg wants to merge 2 commits into
ponylang:masterfrom
zevlg:connection-fail-error-expose
Closed

Exposes socket connection error into pony level#1520
zevlg wants to merge 2 commits into
ponylang:masterfrom
zevlg:connection-fail-error-expose

Conversation

@zevlg

@zevlg zevlg commented Jan 18, 2017

Copy link
Copy Markdown
Contributor
  • net/tcp_connection_notify: added connect_error method -
    extended version of connect_failed

Exposes socket connection error into pony level

* net/tcp_connection_notify: added `connect_error` method -
  extended version of `connect_failed`
@SeanTAllen

Copy link
Copy Markdown
Member

This fails on Windows.

@chalcolith

chalcolith commented Jan 18, 2017

Copy link
Copy Markdown
Member

Change src/libponyrt/lang/socket.c line 737 to the following to build on Windows:

if(getsockopt((SOCKET)fd, SOL_SOCKET, SO_ERROR, (char*)&sockerr, &errlen))

On Windows getsockopt() takes a char * instead of a void * there, but the behaviour is the same (see https://msdn.microsoft.com/en-us/library/windows/desktop/ms738544(v=vs.85).aspx)

[fix] lang/socket.c (pony_os_socket_error): cast optval to suite
      Windows `getsockopt()` API
@zevlg

zevlg commented Jan 19, 2017

Copy link
Copy Markdown
Contributor Author

Fixed, should work on Windows

@jemc

jemc commented Jan 20, 2017

Copy link
Copy Markdown
Member

It looks like this breaks code which depends on connect_failed, but that method hasn't been removed from the interface.

Also, I think a change like this needs to go through the RFC process, to get community input - especially for a breaking change like this.

@zevlg

zevlg commented Jan 21, 2017

Copy link
Copy Markdown
Contributor Author

It does not break existing code with connect_failed, since default implementation for connect_error calls cannect_failed

Btw it could be useful to have something like \deprecated\ annotation to mark API changes while keeping compatibility

@zevlg

zevlg commented Jan 21, 2017

Copy link
Copy Markdown
Contributor Author

I'm very new to pony, don't know the procedures needed to accept the code. I see there is RFC 23 already exist with related subject

@jemc

jemc commented Jan 23, 2017

Copy link
Copy Markdown
Member

I'm very new to pony, don't know the procedures needed to accept the code

Sorry, I should have provided a better explanation. For an API change like this, we'd like to see an RFC first, so we can get community input about the best approach. The hope is that with more eyes on the process, coming from different perspectives and thinking about different use cases, we're likely to come to better solutions than individuals will, leading to better and more stable APIs overall.

The process for creating an RFC is described here: https://github.com/ponylang/rfcs/blob/master/README.md

The TLDR is that you create a pull request with the RFC, get community input on the idea and the approach, then get it accepted for work. We try not to have the process be too formal, because we don't want to slow down innovation too much, but we do want to slow things down a little bit to get you and the rest of us thinking seriously about the motivation for the idea, impact of the change and the best way to go about it.

If you need any help figuring out the RFC process, please let us know.


We can discuss more in the RFC, but I personally get the feeling that exposing the errno directly is likely to be too low-level. Ideally, the errors could be expressed somehow in the Pony type system instead of as (platform dependent?) numbers. We do something like this both in the process package and in the files package.

@zevlg

zevlg commented Jan 24, 2017

Copy link
Copy Markdown
Contributor Author

aha, thanks. Will try to compose RFC

@SeanTAllen SeanTAllen force-pushed the master branch 6 times, most recently from 97b1943 to 21d37aa Compare March 11, 2017 14:51
@SeanTAllen

Copy link
Copy Markdown
Member

As the RFC for this is now open, I'm closing this.

ponylang/rfcs#78

@SeanTAllen SeanTAllen closed this Mar 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants