/bitcoin

Fix stale socket rebinding and re-enable python tests for Windows #6590

Merged
merged 2 commits into from Aug 26, 2015

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@theuni @ptschip @Diapolo @laanwj
@theuni
Member

theuni commented Aug 25, 2015

The fix is debatable, but it points out the real issue: when testing with Windows, stale sockets from previous runs cause the current tests' listen sockets to be unavailable, leading to nodes that never sync. I'm sure that translates to real-world issues as well, for example watchdog scripts would likely fail to work as intended. I'm not sure if there's a more proper way to fix the problem for Windows, so I just copied the behavior we already use for Unix.

This may be part of the root cause of #6554, but I'm not sure about that.

With that fixed, these test run fine locally. Note that they do take quite a while to run though, so we might not want to enable them on Travis for every PR/push.

theuni added some commits Aug 20, 2015

@theuni
net: Set SO_REUSEADDR for Windows too 
When running the rpc tests in Wine, nodes often fail to listen on localhost
due to a stale socket from a previous run. This aligns the behavior with other
platforms.
a193387
@theuni
rpc-tests: re-enable rpc-tests for Windows
bd30c3d

@theuni theuni referenced this pull request Aug 25, 2015

Merged

Enable python regression test for Windows #6548

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Aug 25, 2015

Member

Grr, I forgot to mention here that this is only known to fix issues with running the Windows tests on Linux via Wine. This isn't a substitute for #6548 which aims to fix them when running natively.

Imo it'd be easier to make sure they work with Travis first, so we can ensure that any fixes for native Windows don't cause any regressions.
Member

theuni commented Aug 25, 2015

Grr, I forgot to mention here that this is only known to fix issues with running the Windows tests on Linux via Wine. This isn't a substitute for #6548 which aims to fix them when running natively.

Imo it'd be easier to make sure they work with Travis first, so we can ensure that any fixes for native Windows don't cause any regressions.

@theuni theuni changed the title from Re-enable python tests for Windows to Fix stale socket rebinding and re-enable python tests for Windows Aug 25, 2015

@ptschip

This comment has been minimized.

Show comment
Hide comment
@ptschip

ptschip Aug 25, 2015

Contributor

Cory,

From you pull request I downloaded and built on native windows and can
confirm that those changes do "not" fix issue 6554

however,

I saw that the tests for windows, "on linux" did pass, so that was good.

The only request I have is that you try running that build with your
changes at least 2 more times. The reason is that the issue, at least
with 6554, is only intermittent and happens roughly 30 to 50% of the time.

On 25/08/2015 9:23 AM, Cory Fields wrote:

Grr, I forgot to mention here that this is only known to fix issues
with running the Windows tests on Linux via Wine. This isn't a
substitute for #6548 #6548
which aims to fix them when running natively.

Imo it'd be easier to make sure they work with Travis first, so we can
ensure that any fixes for native Windows don't cause any regressions.

—
Reply to this email directly or view it on GitHub
#6590 (comment).
Contributor

ptschip commented Aug 25, 2015

Cory,

From you pull request I downloaded and built on native windows and can
confirm that those changes do "not" fix issue 6554

however,

I saw that the tests for windows, "on linux" did pass, so that was good.

The only request I have is that you try running that build with your
changes at least 2 more times. The reason is that the issue, at least
with 6554, is only intermittent and happens roughly 30 to 50% of the time.

On 25/08/2015 9:23 AM, Cory Fields wrote:

Grr, I forgot to mention here that this is only known to fix issues
with running the Windows tests on Linux via Wine. This isn't a
substitute for #6548 #6548
which aims to fix them when running natively.

Imo it'd be easier to make sure they work with Travis first, so we can
ensure that any fixes for native Windows don't cause any regressions.

—
Reply to this email directly or view it on GitHub
#6590 (comment).
@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Aug 25, 2015

Member

The issue that this fixes is 100% reproducible, it happens every time without the fix, and never with it. I'll admit that I'm puzzled as to why it doesn't happen on native Windows, though. Maybe it has to do with how quickly the processes are spun up.

I think it's safe to say they're separate issues.
Member

theuni commented Aug 25, 2015

The issue that this fixes is 100% reproducible, it happens every time without the fix, and never with it. I'll admit that I'm puzzled as to why it doesn't happen on native Windows, though. Maybe it has to do with how quickly the processes are spun up.

I think it's safe to say they're separate issues.
@ptschip

This comment has been minimized.

Show comment
Hide comment
@ptschip

ptschip Aug 25, 2015

Contributor

That's great...when this fix get's merged I'll make the changes to the "enable python scripts for windows" without the travis.yml updates.
Contributor

ptschip commented Aug 25, 2015

That's great...when this fix get's merged I'll make the changes to the "enable python scripts for windows" without the travis.yml updates.
@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Aug 25, 2015

IMHO this change wont hurt even when applied to native Windows. I'm going to integrate it into my local build and will run my 2 partially online nodes with it applied and see what happens ;). Perhaps there are other ifdef cases that apply settings/state for Linux but not Windows...

Diapolo commented Aug 25, 2015

IMHO this change wont hurt even when applied to native Windows. I'm going to integrate it into my local build and will run my 2 partially online nodes with it applied and see what happens ;). Perhaps there are other ifdef cases that apply settings/state for Linux but not Windows...
src/net.cpp
setsockopt(hListenSocket, SOL_SOCKET, SO_REUSEADDR, (void*)&nOne, sizeof(int));
+#else
+ setsockopt(hListenSocket, SOL_SOCKET, SO_REUSEADDR, (const char*)&nOne, sizeof(int));

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Aug 26, 2015

Member

Verified that this is the correct type: type of SO_REUSEADDR's value is a BOOL according to MS' API docs, and BOOL is a typedef of int. nOne is an int.
@laanwj

laanwj Aug 26, 2015

Member

Verified that this is the correct type: type of SO_REUSEADDR's value is a BOOL according to MS' API docs, and BOOL is a typedef of int. nOne is an int.
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Aug 26, 2015

Member

utACK
Member

laanwj commented Aug 26, 2015

utACK

@laanwj laanwj merged commit bd30c3d into bitcoin:master Aug 26, 2015
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Aug 26, 2015

@laanwj
Merge pull request #6590 
bd30c3d rpc-tests: re-enable rpc-tests for Windows (Cory Fields)
a193387 net: Set SO_REUSEADDR for Windows too (Cory Fields)
Verified
This commit was signed with a verified signature.
@laanwj laanwj Wladimir J. van der Laan
GPG key ID: 74810B012346C9A6 Learn about signing commits
981fd92

@theuni theuni referenced this pull request Aug 29, 2015

Merged

libevent-based http server #5677

5 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment