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

[Breaking Change Request] Destroyed/Upgraded Sockets now throw when accessing socket options #40702

Closed
sortie opened this issue Feb 20, 2020 · 7 comments
Assignees
Labels
area-sdk Use area-sdk for general purpose SDK issues (packaging, distribution, …). breaking-change-request This tracks requests for feedback on breaking changes NNBD Issues related to NNBD Release

Comments

@sortie
Copy link
Contributor

sortie commented Feb 20, 2020

Intended change

The Socket class will now throw a SocketException if the socket has been explicitly destroyed or upgraded to a secure socket upon setting or getting socket options. Previously setting a socket option would be ignored and getting a socket option would return null.

Rationale

The non-nullable-by-default migration required making subtle changes to some dart:io semantics in order to provide a better API. The Socket.getRawOption() method never returns null except if the socket had been destroyed or upgraded to a secure socket, after which the socket object should not be used. It will never return null for all legitimate uses, so to provide a better API when Dart is null-safe, we would like to change the API to never return null and instead throw a SocketException in this case. This behavior is consistent with the Socket.port, Socket.address, Socket.remotePort, and Socket.remoteAddress getters that also throw a SocketException when the socket has been destroyed or upgraded to a secure socket.

While here, the Socket.setOption() and Socket.setRawOption() methods are changed to also throw for consistency in this case instead of silently not setting the option.

Expected impact

Socket objects are not supposed to be used after destroy() has been called on them or after they've been upgraded to a secure socket. Therefore code shouldn't been trying to set or get socket options at that time and it's not expected any code will break.

Steps for mitigation

Affected code should be changed to not set or get socket options after sockets has been explicitly destroyed or upgraded to a secure socket. Alternatively such code can catch the SocketException.

Implementation

This change was implemented in https://dart-review.googlesource.com/c/sdk/+/134328.

cc @franklinyow @mit-mit @lrhn @athomas

@sortie sortie added area-sdk Use area-sdk for general purpose SDK issues (packaging, distribution, …). breaking-change-request This tracks requests for feedback on breaking changes NNBD Issues related to NNBD Release labels Feb 20, 2020
dart-bot pushed a commit that referenced this issue Feb 21, 2020
This is a breaking change. #40702

The change originally landed in fddbc53
but it was decided to file a breaking change request about this change
as it is observable.

Change-Id: Ie9b045f3abec858486d8efb335eedf2c68e78951
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/136583
Reviewed-by: Lasse R.H. Nielsen <lrn@google.com>
@franklinyow
Copy link
Contributor

cc @Hixie @matanlurey @dgrove @vsmenon for review and approval.

@matanlurey
Copy link
Contributor

No input

@franklinyow
Copy link
Contributor

Ping @Hixie @vsmenon

@vsmenon
Copy link
Member

vsmenon commented Feb 26, 2020

lgtm

1 similar comment
@Hixie
Copy link
Contributor

Hixie commented Feb 28, 2020

lgtm

@franklinyow
Copy link
Contributor

Approved

@franklinyow franklinyow assigned sortie and unassigned franklinyow Feb 29, 2020
@sortie
Copy link
Contributor Author

sortie commented Mar 6, 2020

The change has already been landed.

@sortie sortie closed this as completed Mar 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-sdk Use area-sdk for general purpose SDK issues (packaging, distribution, …). breaking-change-request This tracks requests for feedback on breaking changes NNBD Issues related to NNBD Release
Projects
None yet
Development

No branches or pull requests

5 participants