-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Merge pull request #914 from marci4/Issue900 #914
Conversation
Currently looking for some input on how to test it (see #900) for more! |
src/main/java/org/java_websocket/exceptions/WrappedIOException.java
Outdated
Show resolved
Hide resolved
Please can this be merged? |
@MichaelPote feel free to write a test for this issue or contribute to the project in another way! |
@marci4 I've looked through the pull request and the test you wrote, but unfortunately I lack the context and understanding to contribute. It seems to me like you've provided a fix for the issue and written a test (https://gist.github.com/marci4/6b47a9475d440a3dcd0d5212095d5ab3) already? |
@MichaelPote no, the test is not finished and I have no real idea on how to get this issue tested. The gist is just how far I got |
Overwrite channel to always throw IOException
Adjust year
@PhilipRoman could you do a review in my changes. I found a way to test this issue! |
|
||
@Override | ||
public boolean isNeedWrite() { | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this return true? Looking at
Java-WebSocket/src/main/java/org/java_websocket/SocketChannelIOHelper.java
Lines 88 to 90 in ec0fa03
if( c.isNeedWrite() ) { | |
c.writeMore(); | |
} |
it seems that the writeMore()
isn't actually called in this test. Or are you relying on other methods to throw the exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, good point, I relied on the normal read/write call. Changing readMore/writeMore to true will however ensure better behavior
public class Issue900Test { | ||
|
||
CountDownLatch countServerDownLatch = new CountDownLatch(1); | ||
CountDownLatch countCloseCallsDownLatch = new CountDownLatch(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These variables have confusing names. I think serverStartedLatch
and serverStoppedLatch
would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Description
Due to refactoring when an IOException happens, the WebSocket was always null and therefore close was never called on it.
Related Issue
Fixes #900
Motivation and Context
Regression
How Has This Been Tested?
Currently only a manual test
Types of changes
Checklist: