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.

Sign up for GitHub

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

Jump to bottom

provide fix and test for #2790 #2796

Merged
merged 1 commit into from
Jan 14, 2019
Merged

provide fix and test for #2790 #2796

merged 1 commit into from
Jan 14, 2019

Conversation

KowalczykBartek
Copy link
Contributor

@KowalczykBartek KowalczykBartek commented Jan 9, 2019 •
edited
Loading

Hi,
that is initial bug-fix for #2790.

I have one problem with test - because of closing file is asynchronous I had to handle exception in Vert.x exception handler

 vertx.exceptionHandler(exception -> fail());

and also schedule completion to allow vert.x first catch exception and fail test.

vertx.setTimer(10, val -> complete());

do you maybe have other idea how to test that case with file-closed case (my seems really ... bad) ?

@vietj
Copy link
Member

vietj commented Jan 10, 2019

looks good, but can we have the same test using the FakeStream instead that reproduces the same asynchrony than AsyncFile ?

@KowalczykBartek
Copy link
Contributor Author

I will try to replace async-file with FakeStream

@vietj
Copy link
Member

vietj commented Jan 10, 2019

thanks

@KowalczykBartek KowalczykBartek force-pushed the bug-fix/2790 branch 2 times, most recently from 0b09668 to 3d57481 Compare January 10, 2019 21:22
@KowalczykBartek
Copy link
Contributor Author

I removed that async-file dependency, so just added part that ensures this "return" statement in existing test, I think that it is enough :)

vietj
vietj reviewed Jan 11, 2019
View reviewed changes
src/test/java/io/vertx/core/parsetools/JsonParserTest.java Outdated
FakeStream stream = new FakeStream();
FakeStream stream = new FakeStream(){
//regression check for #2790 - ensure that by accident resume method is not called.
@Override
Copy link
Member

Choose a reason for hiding this comment

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

rather add on FakeStream a pauseCount / resumeCount that are incremented when those methods are called and check the counters are == 0 at the end of the test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@KowalczykBartek KowalczykBartek force-pushed the bug-fix/2790 branch 2 times, most recently from 305ef34 to 3ca9285 Compare January 11, 2019 08:19
@KowalczykBartek
Copy link
Contributor Author 

Tests in error: 
  Http1xTest.testHttpServerWithIdleTimeoutSendChunkedFile Âť Vertx Connection was...

failed - but I think it it not related to my change.

@vietj
Copy link
Member

vietj commented Jan 11, 2019

@KowalczykBartek yes some tests fails due to races sometimes

vietj
vietj reviewed Jan 11, 2019
View reviewed changes
src/test/java/io/vertx/core/parsetools/FakeStream.java Outdated
@@ -24,6 +24,8 @@
private Handler<Buffer> eventHandler;
private Handler<Void> endHandler;
private Handler<Throwable> exceptionHandler;
int pauseCount;
Copy link
Member

Choose a reason for hiding this comment

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

make it private and add a pauseCount() accessor

Copy link
Contributor Author

@KowalczykBartek KowalczykBartek Jan 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

done

btw, thanks for review time - I guess you could safe half of time writing this yourself :D

vietj
vietj reviewed Jan 11, 2019
View reviewed changes
src/test/java/io/vertx/core/parsetools/FakeStream.java Outdated
@@ -89,4 +93,12 @@ void fail(Throwable err) {
void end() {
endHandler.handle(null);
}

public int getPauseCount() {
Copy link
Member

Choose a reason for hiding this comment

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

rather pauseCount() and resumeCount()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

vietj
vietj reviewed Jan 13, 2019
View reviewed changes
src/test/java/io/vertx/core/parsetools/FakeStream.java
@@ -89,4 +93,12 @@ void fail(Throwable err) {
void end() {
endHandler.handle(null);
}

public int pauseCount() {
Copy link
Member

Choose a reason for hiding this comment

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

make both synchronized and I think it will be fine. (this class can be used in multithreaded tests)

Copy link
Contributor Author

@KowalczykBartek KowalczykBartek Jan 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

in such case, instead of int - AtomicInteger should be provided, because
a. There is race during value incrementation
b. there is no happens-before relationship between adding and reading.

and honestly I don't get what synchronized give us here.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine, we are not trying to remove race conditions so we don't need happens-before, just ensure safe publication of the value, these values are likely to be used at the end of the test.

Copy link
Contributor Author

@KowalczykBartek KowalczykBartek Jan 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

synchronized keyword do not ensure safe publication,this could be achieved with volatile - or do I miss some part of JMM ?

Copy link
Member

Choose a reason for hiding this comment

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

It does when the value was stored under synchronization, so we just need to ensure that we use the same monitor and synchronize when we store the value.

Copy link
Contributor Author

@KowalczykBartek KowalczykBartek Jan 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@vietj where is corresponding synchronize on writing data (on the same monitor (so instance of FakeStream)) ? to make this example correct, something have to write and read using FakeStream instance monitor.

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 added volatile to both pauseCount and resumeCount - this for sure will ensure visibility correctness between any thread reading and writing (of course do not prevent against races). I could not find example that uses FakeStream in multithreaded environment, so, I could not verify if synchronization keyword could help.
Is that ok ?

Copy link
Member

Choose a reason for hiding this comment

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

@KowalczykBartek it is used in rx-java module I haven't checked whether tests are multi-threaded though but there are chances there are.

Copy link
Contributor Author

@KowalczykBartek KowalczykBartek Jan 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

in rx-java module I see that

io.vertx.test.fakestream.Fakestream

and that is true, that this class have synchronization in place. But we are talking about

io.vertx.core.parsetools.FakeStream

in this case, there is no happens-before.

Copy link
Member

Choose a reason for hiding this comment

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

ah you are right, so just leave it as is and in the future we'll try to use a single FakeStream class in vertx test classes

@KowalczykBartek
This is a combination of 2 commits.
0192a93 
Provide fix and test for eclipse-vertx#2790

Signed-off-by: Kowalczyk <bkowalczyyk@gmail.com>

Signed-off-by: Kowalczyk <bkowalczyyk@gmail.com>
@vietj vietj merged commit 6ae88eb into eclipse-vertx:3.6 Jan 14, 2019
@vietj
Copy link
Member

vietj commented Jan 14, 2019

thanks for the contribution @KowalczykBartek

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vietj vietj vietj left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@KowalczykBartek @vietj