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

Set the actual local port number to btrace.port system property #461

Merged
merged 1 commit into from
Apr 10, 2021
Merged

Set the actual local port number to btrace.port system property #461

merged 1 commit into from
Apr 10, 2021

Conversation

trustin
Copy link
Contributor

@trustin trustin commented Apr 4, 2021

Motivation:

BTrace agent currently sets the port number given by a user to
btrace.port system property as-is. This behavior can be a problem
when:

  • The agent actually failed to bind to the given port, because a user
    may still attempt to connect to the port number specified in
    btrace.port, where other server may be running already there.
  • A user cannot know the actual port where BTrace agent is running when
    the user specified 0 as the port number, because we only know the
    actual port number after creating a server socket on it.

Modifications:

  • Set the actual local port number after creating a ServerSocket so
    that a user can later figure out the actual port number even if the
    user specified 0 as the port number.

Result:

  • A user can know the actual local port number, even when the user
    specified 0.
  • A user can know the agent server socket is not open easily by checking
    the existence of btrace.port System property.

Motivation:

BTrace agent currently sets the port number given by a user to
`btrace.port` system property as-is. This behavior can be a problem
when:

- The agent actually failed to bind to the given port, because a user
  may still attempt to connect to the port number specified in
  `btrace.port`, where other server may be running already there.
- A user cannot know the actual port where BTrace agent is running when
  the user specified `0` as the port number, because we only know the
  actual port number *after* creating a server socket on it.

Modifications:

- Set the actual local port number after creating a `ServerSocket` so
  that a user can later figure out the actual port number even if the
  user specified `0` as the port number.

Result:

- A user can know the actual local port number, even when the user
  specified `0`.
- A user can know the agent server socket is not open easily by checking
  the existence of `btrace.port` System property.
@jbachorik
Copy link
Collaborator

Can you, please, make sure you have OCA signed? https://oca.opensource.oracle.com/

@trustin
Copy link
Contributor Author

trustin commented Apr 5, 2021

I can't sign the OCA because it fails saying "You can't submit a new request unless the one that is pending signature is resumed or withdrawn." I indeed cancelled the DocuSign session because I realized my information in the Oracle account was outdated (wrong employer name and address). Now that I updated my information, but I can't sign anymore. 🤔

@trustin
Copy link
Contributor Author

trustin commented Apr 5, 2021

Could someone clean up my state so I can try again?

@trustin
Copy link
Contributor Author

trustin commented Apr 5, 2021

Alternatively, how about making the proposed change by yourself? It's just a one liner after all. I hereby release this changeset to the public domain (Creative Commons Zero 1.0).

@trustin
Copy link
Contributor Author

trustin commented Apr 9, 2021

@jbachorik I've just figured out how to do it by myself and signed the OCA. Could you please merge this and make it part of the next releases?

@jbachorik
Copy link
Collaborator

Great, I will merge this as soon as you appear in the list of signatories. I am afraid it can take a couple of days :(

@jbachorik jbachorik merged commit 8d882e7 into btraceio:develop Apr 10, 2021
@trustin trustin deleted the actual_local_port branch April 12, 2021 01:42
@trustin
Copy link
Contributor Author

trustin commented Apr 12, 2021

Thanks, @jbachorik ! Looking forward to the next release. 🙇

This pull request was closed.
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.

2 participants