Skip to main content
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [jaxrs-dev] [External] : Rx TCK Tests

On Thu, Mar 24, 2022 at 1:46 PM Jan Supol <jan.supol@xxxxxxxxxx> wrote:
Hi James,
> race condition to me to assume the future is done at this stage

It is doing the opposite.

Oh wow, you're absolutely right Jan. I apologize. I kept reading the assertion message too and it didn't even click!
 
The test checks that the future is NOT done, yet, i.e. that the rx code is nonblocking, running the HTTP query in a new thread, passing the main thread directly to Future.isDone(). GitHub Actions seems to be doing some funny stuff with threads though. The code might be done better, adding some countdown latch now when the client is run one the same JVM as the server, unlike the EE 8 days.

I wouldn't be surprised if GitHub Actions does something funny with threads. I've definitely had other issues running tests on it, but it's just so convenient :) Using a countdown latch does seem reasonable. As you pointed out, things may end up completing faster so I do think it makes sense for some test improvements there.
 

-- Jan
From: jaxrs-dev <jaxrs-dev-bounces@xxxxxxxxxxx> on behalf of James Perkins <jperkins@xxxxxxxxxx>
Sent: Thursday, March 24, 2022 9:24 PM
To: jaxrs-dev <jaxrs-dev@xxxxxxxxxxx>
Subject: [External] : [jaxrs-dev] Rx TCK Tests
 
Hello All,
I'm curious if anyone has some background on tests like [1]. It creates a CompletionStageRxInvoker, starts a request and converts the CompletationStage to a Future. All that seems fine, except when it gets to [2] where it checks if the future is done. It seems like a bit of a race condition to me to assume the future is done at this stage. While I've seen no issues running this locally or on most CI environments I've run it on, I do see issues on GitHub Actions which is failing several of the tests like this. It's definitely a timing issue because they don't all consistently fail, but it always fails at least one of them because the future is not yet done.

My assumption is the Future.isDone() check is to avoid the possible hang on Future.get(). However, it seems we could easily change that to something like Future.get(configurableTimeout, TimeUnit.SECONDS). Does this seem reasonable? I'm happy to contribute a patch if it seems reasonable.

Just to note too I absolutely do not think this should block any current 3.1 efforts :) The tests have been like this for a long time and for me it's ONLY failing on GitHub Actions.

Thanks in advance!

_______________________________________________
jaxrs-dev mailing list
jaxrs-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jaxrs-dev


--
James R. Perkins
JBoss by Red Hat

Back to the top