Prevent browser to be created and unused in case a short session-request-timeout is set

[bhecquet]

Description

This PR corrects the issue #12832 where driver sessions may be created, but unused if driver takes long time to create
In out case, session request timeout is set to 30 secs, as we have several hubs for redundancy, and we do not want to be stuck with a single hub on driver creation.

Motivation and Context

When hubs are loaded, slots take some time to free, so that a new session can be created, and in this case, request session timeout may be raised during driver creation. As a result, driver is created on node, but client has received a SessionNotCreatedException

In this PR, after a driver is successfuly created, we check the session queue to see if the session request has expired or not. If it has expired (not in requests list or has reached timeout), then, we notify the node to stop the session immediately to release the slot

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

I would have liked to add a unit test for LocalDistributor part, but could not find a proper way to do this. If you have any idea ...

Fixes #12832

[joerg1985]

I think there might be an easier way to handle this cases, just return a boolean in NewSessionQueue.complete?

  /**
   * @return true if the queue accepted the session/exception, false if the session was not accepted and should be terminated
   * (usually when the session request hit the timeout before and was rejected)
   */
  public abstract boolean complete(
      RequestId reqId, Either<SessionNotCreatedException, CreateSessionResponse> result);

The LocalDistributor could then handle it this way:

        boolean accepted = sessionQueue.complete(reqId, response);
        attributeMap.put("request.accepted", accepted);

        if (!accepted && response.isRight()) {
          terminateSession(response.right().getSession().getId());
        }

Only a idea, i am not telling the current PR way is bad :D

[bhecquet]

I agree with you, there will be less changes in code
Will change my PR accordingly

[diemol]

Thank you for your PR.

I am missing a test that also checks that the session is removed from the Node. You could add it in DistributorTest.

[codecov-commenter]

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (d8b9333) 56.49% compared to head (f8e9045) 56.49%.
Report is 1 commits behind head on trunk.

❗ Current head f8e9045 differs from pull request most recent head 7d2def6. Consider uploading reports for the commit 7d2def6 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##            trunk   #12848   +/-   ##
=======================================
  Coverage   56.49%   56.49%           
=======================================
  Files          86       86           
  Lines        5255     5255           
  Branches      187      187           
=======================================
  Hits         2969     2969           
  Misses       2099     2099           
  Partials      187      187           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pujagani]

@bhecquet Probably to verify the Distributor changes via tests, essentially to check if this works end to end as expected, have a look at the setup in https://github.com/SeleniumHQ/selenium/blob/trunk/java/test/org/openqa/selenium/grid/router/SessionQueueGridTest.java. The end-to-end tests there try to valid different scenarios involving the new session queue. Maybe add to the same test or create another test class with a similar setup to emulate the motivation the changes solve.

[diemol]

Thank you, @bhecquet!