KNOX-1997: Adding changes to buffer messages from backend in onMessag… by goelrajat · Pull Request #143 · apache/knox

goelrajat · 2019-09-06T08:29:05Z

What changes were proposed in this pull request?

Adding changes to buffer messages from backend in onMessageText() if frontend session related data structures have not been setup i.e. remote is null. Message buffer will be flushed when remote is set by other thread executing onWebSocketConnect() API. To synchronize reading/flushing buffer, added a lock

How was this patch tested?

Race condition was easily reproducible with our service UI and backedn when integrated with Knox. After this change, I no longer see the NullPointerException. Since this is a race condition, its hard to add a unit test case for it. In my case, the race condition was getting hit when service backend used to send a data frame immediately after opening a websocket connection but Knox hadn't setup the connection related data structures. So I added a test case for scenario where send a message to frontend while opening a websocket.

risdenk · 2019-09-06T14:14:27Z

So I don't know this code, but it seems weird to me that getRemote returns null at all. It looks like the remote null check is fixing the symptom not the root cause. Is it possible to make getRemote block and never return null? I think that would avoid having to buffer messages at all?

What I observed that remote was being created in other thread which was executing onWebSocketConnect() while this current thread received a message. This might be an limitation with Jetty itself, check: signalapp/Signal-Desktop#1840.

I checked jetty examples and even there the server is using locks before accessing remote, as mentioned in the above link.

I prefer to avoid locks if they can be avoided. Instead of buffering can we wait for a configurable amount of time to see if the remote object is created before quitting ? This should not be an issue IMO since this should be on a different thread than other connections.

I tried adding a fixed delay of few seconds in onMessageText() API but I was still seeing the issue in some cases. So adding a fixed configurable delay will not work. I aslo want to avoid locks as they incur a performance overhead. IF you have any suggestions on how can locks be avoided, let me know. Thanks.

moresandeep

Thanks @goelrajat, the patch is on track, we requested few changes. The concerning one is about Locks, if we can avoid them that would be great. there are callback functions, not sure of we can leverage them but it would be good to try them out ! Thanks again for the patch and for finding and troubleshooting it !

moresandeep · 2019-09-06T15:39:09Z

I don't like a locks here, can they be avoided, e.g. by using configurable wait in public void onMessageText(String message, Object session) { not sure if this is optimal, this is the first thing that came to my mind.

I tried adding a fixed delay of few seconds but I was still seeing the issue in some cases. So delay is unpredictable.

moresandeep · 2019-09-06T15:45:21Z

Lock in a callback seems like a bad idea, especially the lock is dependent on the creation of the front end session. Is there a way to avoid locks ?

I could not think of any other way, haven't worked much with Jetty. If you can provide any pointers, I will be happy to try things out.

moresandeep · 2019-09-06T15:56:32Z

I prefer to avoid locks if they can be avoided. Instead of buffering can we wait for a configurable amount of time to see if the remote object is created before quitting ? This should not be an issue IMO since this should be on a different thread than other connections.

moresandeep · 2019-09-06T16:07:40Z

This should probably go inside of try{ in case getRemote() throws an exception.

goelrajat · 2019-09-19T06:55:57Z

Any updates on whether this PR can be merged or something more needs to be done here.

risdenk · 2019-09-19T14:42:26Z

@goelrajat you might have to rebase your commit on latest master to get Travis builds to pass. Just pushed a build fix a few minutes ago. I'll leave it to @smolnar82 and @moresandeep to review

goelrajat · 2019-09-20T10:26:09Z

@risdenk Rebase has no impact:
bash-3.2$ git pull origin master
From https://github.com/goelrajat/knox

branch master -> FETCH_HEAD
Already up to date.
bash-3.2$ git rebase master
Current branch KNOX-1997 is up to date.
bash-3.2$

smolnar82 · 2019-09-20T11:39:40Z

https://github.com/goelrajat/knox

Your master is behind apache/knox by 21 commits:

First, you need to pull changes from apache/master to your master then you can rebase.

goelrajat · 2019-09-20T12:11:06Z

@smolnar82 Done. Thanks

smolnar82

LGTM

smolnar82 · 2019-09-20T12:18:43Z

One more thing: since you added a new configuration in gateway-site.xml it should be documented (even if it's optional). Could you please open another JIRA to modify the user guide?
Thanks!

smolnar82 · 2019-09-20T12:29:57Z

@goelrajat - tests are failing:

[ERROR] Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 1.218 s <<< FAILURE! - in org.apache.knox.gateway.websockets.WebsocketServerInitiatedMessageTest
[ERROR] testGatewayRewriteHttpEcho(org.apache.knox.gateway.websockets.WebsocketServerInitiatedMessageTest)  Time elapsed: 1.063 s  <<< ERROR!
java.util.concurrent.TimeoutException: Timed out reading message from queue
	at org.apache.knox.gateway.websockets.WebsocketServerInitiatedMessageTest.testGatewayRewriteHttpEcho(WebsocketServerInitiatedMessageTest.java:166)```

goelrajat · 2019-09-20T12:46:02Z

@smolnar82 Test case is passing on my local build setup. However, I have increased timeout now and updated the test case. Hopefully, should work now.

Raised a ticket for documentation: https://issues.apache.org/jira/browse/KNOX-2019

smolnar82 · 2019-09-20T13:21:05Z

@goelrajat - tests are failing again:

[ERROR] COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
[ERROR] /src/gateway-test-release-utils/src/main/java/org/apache/knox/gateway/GatewayTestConfig.java:[38,8] org.apache.knox.gateway.GatewayTestConfig is not abstract and does not override abstract method getWebsocketMaxWaitBufferCount() in org.apache.knox.gateway.config.GatewayConfig

Before you submit a new patch could you please run the following command locally:
mvn clean -T1C verify -Prelease,package

risdenk

@moresandeep / @smolnar82 any other comments about this?

smolnar82 · 2019-09-23T14:12:46Z

Nope; works for me now!

moresandeep

@goelrajat apologies for the delay, I think I am ok with the locks. I can't think of a better way to do it right now and don't want to hold up the patch for it. I requested few changes on the unit tests, once we have that the patch is good to go (I'll do some internal testing as well to make sure we don't break anything).
Thanks again !

goelrajat · 2019-09-27T09:04:19Z

Travis build failed due to failure in test case WebsocketServerInitiatedMessageTest, however the same test case is passing on my local setup. I tried multiple times but it works on my side.

bash-3.2$ mvn test -Dtest=org.apache.knox.gateway.websockets.WebsocketServerInitiatedMessageTest
[INFO] Running org.apache.knox.gateway.websockets.WebsocketServerInitiatedMessageTest
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.504 s - in org.apache.knox.gateway.websockets.WebsocketServerInitiatedMessageTest
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------

Can someone please check?

…eText() if frontend session related data structures have not been setup i.e. remote is null. Message buffer will be flushed when remote is set by other thread executing onWebSocketConnect() API. To synchronise reading/flushing buffer, added a lock

…gument to constructor

…assing on local setup

goelrajat · 2019-09-30T05:48:26Z

Test case changes have been done. Please check and merge.

smolnar82 · 2019-09-30T07:45:35Z

LGTM +1

goelrajat · 2019-10-03T12:40:26Z

@moresandeep Please check and merge this one as well.

goelrajat · 2019-10-11T08:36:49Z

@moresandeep Can you please merge this PR? I want to raise another PR for KNOX-2004 and its changes are dependent on this PR.

moresandeep · 2019-10-11T13:49:20Z

@moresandeep Can you please merge this PR? I want to raise another PR for KNOX-2004 and its changes are dependent on this PR.

@goelrajat I will get to it probably mid next week, since this is pretty involved and touches a bunch of things I would like to test it thoroughly.

risdenk · 2019-10-30T18:21:11Z

@moresandeep any update here?

moresandeep · 2019-10-30T20:58:00Z

Still underwater, hoping to find some time by the end of this week.

goelrajat · 2019-11-13T07:27:05Z

Any updates on this one please? When can we merge this ? Its been idle for too long now.

moresandeep

Looks good, sorry for the delay in reviewing.

smolnar82 reviewed Sep 6, 2019

View reviewed changes

risdenk requested changes Sep 6, 2019

View reviewed changes

risdenk requested a review from moresandeep September 6, 2019 14:40

moresandeep requested changes Sep 6, 2019

View reviewed changes

smolnar82 requested changes Sep 9, 2019

View reviewed changes

goelrajat requested a review from moresandeep September 19, 2019 06:53

goelrajat force-pushed the KNOX-1997 branch from aa831f4 to 990b36c Compare September 20, 2019 12:07

smolnar82 approved these changes Sep 20, 2019

View reviewed changes

risdenk approved these changes Sep 23, 2019

View reviewed changes

risdenk self-assigned this Sep 23, 2019

moresandeep requested changes Sep 24, 2019

View reviewed changes

risdenk assigned moresandeep and unassigned risdenk Sep 25, 2019

rajat.goel added 6 commits September 27, 2019 14:44

KNOX-1997: Minor code review fixes

cb75fdc

KNOX-1997: More code review fixes

4d9531a

KNOX-1997: More code review fixes. Passig gateway config object as ar…

3a4244a

…gument to constructor

KNOX-1997: Increasing timeout as test case failing on CI server but p…

de4a50c

…assing on local setup

KNOX-1997: Increasing timeout as test case failing on CI server but p…

305e149

…assing on local setup

KNOX-1997: Refactoring test cases

6dcfdcb

goelrajat force-pushed the KNOX-1997 branch from 1387489 to 6dcfdcb Compare September 27, 2019 09:15

KNOX-1997: Last try for fixing build failure

b3278e4

Rajat Goel added 2 commits October 3, 2019 17:33

Merge branch 'master' into KNOX-1997

d020e83

KNOX-1997: Fixing local test failure after merge

7135afa

moresandeep approved these changes Nov 13, 2019

View reviewed changes

moresandeep merged commit 96587a4 into apache:master Nov 13, 2019

goelrajat deleted the KNOX-1997 branch November 22, 2019 15:40

Conversation

goelrajat commented Sep 6, 2019

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goelrajat Sep 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moresandeep left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

goelrajat commented Sep 19, 2019

Uh oh!

risdenk commented Sep 19, 2019

Uh oh!

goelrajat commented Sep 20, 2019

Uh oh!

smolnar82 commented Sep 20, 2019

Uh oh!

goelrajat commented Sep 20, 2019

Uh oh!

smolnar82 left a comment

Choose a reason for hiding this comment

Uh oh!

smolnar82 commented Sep 20, 2019

Uh oh!

smolnar82 commented Sep 20, 2019

Uh oh!

goelrajat commented Sep 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smolnar82 commented Sep 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

risdenk left a comment

Choose a reason for hiding this comment

Uh oh!

smolnar82 commented Sep 23, 2019

Uh oh!

moresandeep left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

goelrajat commented Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

goelrajat commented Sep 30, 2019

goelrajat Sep 9, 2019 •

edited

Loading

goelrajat commented Sep 20, 2019 •

edited

Loading

smolnar82 commented Sep 20, 2019 •

edited

Loading

goelrajat commented Sep 27, 2019 •

edited

Loading

goelrajat commented Oct 11, 2019 •

edited

Loading