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

Ensure local addresses aren't null #31440

Merged
merged 13 commits into from
Jun 21, 2018

Conversation

Tim-Brooks
Copy link
Contributor

@Tim-Brooks Tim-Brooks commented Jun 19, 2018

Currently we set local addresses on the creation time of a NioChannel.
However, this may return null as the local address may not have been
set yet. An example is the local address has not been set on a client
channel as the connection process is not yet complete.

This PR modifies the getter to set the local field if it is currently null.

Currently we set local and remote addresses on the creation time of a
NioChannel. However, these may return null as they may not have been
set yet. An example is the local address has not been set on a client
channels as the connection process is not yet complete.

This PR modifies the getters to set the local and remote address fields
if they are currently null.
@Tim-Brooks Tim-Brooks added >non-issue review :Distributed/Network Http and internode communication implementations v7.0.0 labels Jun 19, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@Tim-Brooks
Copy link
Contributor Author

Tim-Brooks commented Jun 19, 2018

get*Address calls made to the raw Socket or ServerSocket will return the value even after the channel is closed. So this PR prefers that method of accessing the address.

However, in the case of getRemoteAddress Socket will not return the address (it will return null) if the connection is not complete. Whereas the nio.SocketChannel class will return it as long as the connection process has begun. So that is our backup method of looking for the remote address (but it will throw an exception if the channel has been closed).

@Tim-Brooks
Copy link
Contributor Author

However, in the case of getRemoteAddress Socket will not return the address (it will return null) if the connection is not complete. Whereas the nio.SocketChannel class will return it as long as the connection process has begun. So that is our backup method of looking for the remote address (but it will throw an exception if the channel has been closed).

I actually changed this to initializing remote address in the constructor. This is because the channel should be open and in the connect process when it is passed to the ctor. So the socket channel method to get the remote address should return reliably.

Copy link
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

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

LGTM

remoteAddress = (InetSocketAddress) socketChannel.getRemoteAddress();
} catch (IOException e) {
// This exception will be thrown if the channel is closed. But we do not really care when
// we are just attempting to read the remote address.
Copy link
Contributor

Choose a reason for hiding this comment

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

can we somehow assert that it's closed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that is dangerous because that is not guaranteed in the method contract. It is just from me reading the SocketChannelImpl.java. I changed it to throw an UncheckedIOException (which is caught in the ChannelFactory)

@Tim-Brooks Tim-Brooks changed the title Ensure local and remote addresses aren't null Ensure local addresses aren't null Jun 20, 2018
@Tim-Brooks Tim-Brooks merged commit 86423f9 into elastic:master Jun 21, 2018
dnhatn added a commit that referenced this pull request Jun 23, 2018
* master:
  Add get field mappings to High Level REST API Client (#31423)
  [DOCS] Updates Watcher examples for code testing (#31152)
  TEST: Add bwc recovery tests with synced-flush index
  [DOCS] Move sql to docs (#31474)
  [DOCS] Move monitoring to docs folder (#31477)
  Core: Combine doExecute methods in TransportAction (#31517)
  IndexShard should not return null stats (#31528)
  fix repository update with the same settings but different type (#31458)
  Fix Mockito trying to mock IOException that isn't thrown by method (#31433) (#31527)
  Node selector per client rather than per request (#31471)
  Core: Combine messageRecieved methods in TransportRequestHandler (#31519)
  Upgrade to Lucene 7.4.0. (#31529)
  [ML] Add ML filter update API (#31437)
  Allow multiple unicast host providers (#31509)
  Avoid deprecation warning when running the ML datafeed extractor. (#31463)
  REST high-level client: add simulate pipeline API (#31158)
  Get Mapping API to honour allow_no_indices and ignore_unavailable (#31507)
  [PkiRealm] Invalidate cache on role mappings change (#31510)
  [Security] Check auth scheme case insensitively (#31490)
  In NumberFieldType equals and hashCode, make sure that NumberType is taken into account. (#31514)
  [DOCS] Fix REST tests in SQL docs
  [DOCS] Add code snippet testing in more ML APIs (#31339)
  Core: Remove ThreadPool from base TransportAction (#31492)
  [DOCS] Remove fixed file from build.gradle
  Rename createNewTranslog to fileBasedRecovery (#31508)
  Test: Skip assertion on windows
  [DOCS] Creates field and document level security overview (#30937)
  [DOCS] Significantly improve SQL docs
  [DOCS] Move migration APIs to docs (#31473)
  Core: Convert TransportAction.execute uses to client calls (#31487)
  Return transport addresses from UnicastHostsProvider (#31426)
  Ensure local addresses aren't null (#31440)
  Remove unused generic type for client execute method (#31444)
  Introduce http and tcp server channels (#31446)
@Tim-Brooks Tim-Brooks deleted the fix_get_local_address branch December 10, 2018 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed/Network Http and internode communication implementations >non-issue v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants