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

Issue48 hostnamenotasync #50

Merged
merged 5 commits into from Jan 9, 2016

Conversation

@alexlehm
Copy link
Contributor

commented Jan 5, 2016

fix for issue #48, use executeBlocking for getHostname()

alexlehm added 3 commits Dec 20, 2015
… hostname is passed

between the different classes and is cached in a field in MailClientImpl.
This is still wip, I am not happy with the constructors passing hostname, there should be an easier
way to this.
This has currently no test, I have starting writing a test with EasyMock/PowerMock, but that is not working

Signed-off-by: alexlehm <alexlehm@gmail.com>
(for now decided to go without a test, the powermock stuff is too complicated, otherwise it would be necessary to rewrite the static class to facilitate testing)

Signed-off-by: alexlehm <alexlehm@gmail.com>
Signed-off-by: alexlehm <alexlehm@gmail.com>
@alexlehm alexlehm added the to review label Jan 5, 2016
hostname = Utils.getHostname();
}
}
log.info("hostname: "+hostname);

This comment has been minimized.

Copy link
@vietj

vietj Jan 7, 2016

Contributor

use debug rather than info for logging.

This comment has been minimized.

Copy link
@alexlehm

alexlehm Jan 7, 2016

Author Contributor

forgot to take that out completely, removed now

});
vertx.executeBlocking(
fut -> {
if (hostname == null) {

This comment has been minimized.

Copy link
@cescoffier

cescoffier Jan 7, 2016

Member

(I'm pretty sure to have written that somewhere, but can't find it, so ignore it if it's a duplicate).

the hostname field is accessed from two threads: the event loop and the worker thread. The worker thread should not access it but compute the value and return it to the event loop thread who write and read it)

This comment has been minimized.

Copy link
@vietj

vietj Jan 7, 2016

Contributor

Actually the hostname should not be set in the executeBlocking block but instead resolve the Future, so it should be something like:

if (hostname == null) {
  vertx.<String>executeBlocking(
  fut -> ... fut.complete(value_of_hostname),
  ar -> {
    if (ar.succeeded()) {
      hostName = ar.result();
    } else { ... }
  };)
}

This comment has been minimized.

Copy link
@alexlehm

alexlehm Jan 8, 2016

Author Contributor

changed, i hope I got it right now

set hostname in event thread, not in worker
do not pass vertx to SMTPStarter since its not used

Signed-off-by: alexlehm <alexlehm@gmail.com>
@cescoffier

This comment has been minimized.

There is still an access to the hostname field from the worker thread.

This comment has been minimized.

Copy link
Contributor Author

replied Jan 8, 2016

ok, changed it, can you please take another look

Signed-off-by: alexlehm <alexlehm@gmail.com>
@cescoffier

This comment has been minimized.

Copy link
Member

commented Jan 9, 2016

👍

alexlehm added a commit that referenced this pull request Jan 9, 2016
Issue48 hostnamenotasync merged
fixes #48
@alexlehm alexlehm merged commit cea7c04 into master Jan 9, 2016
@alexlehm alexlehm removed the to review label Jan 9, 2016
@alexlehm alexlehm deleted the issue48_hostnamenotasync branch Jan 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.