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

Async authorization #647

Closed
fdummert opened this Issue Apr 12, 2016 · 4 comments

Comments

Projects
None yet
2 participants
@fdummert

Authentication is usually performed via SecurityPolicy::canHandshake. Normally, authentication involves checking external systems (DB, REST-interface, SAML, ...) for exchanging user credentials with a valid session. Such an authentication process can be time-consuming and currently, it needs to block the thread within canHandshake. As it is always demanded that listeners must not block threads, we have some kind of a conflict here which could maybe be resolved with an asynchronous authentication (e.g. callback style) method.

@sbordet sbordet changed the title from Async authentication to Async authorization Jul 14, 2016

@sbordet

This comment has been minimized.

Show comment
Hide comment
@sbordet

sbordet Jul 14, 2016

Member

Thinking about this, until the handshake is complete, the client cannot send any other message.

Having SecurityPolicy.canHandshake() run asynchronously would just move the problem of I/O blocked threads to a different thread, losing in mechanical sympathy for no real benefit - CometD can't process other messages anyway from that client.

Could be different for SecurityPolicy.can[Create|Subscribe|Publish] and forAuthorizer`.
Running those methods asynchronously would allow CometD to process other messages from the same client, but risks to overload the external system, since CometD won't backpressure messages like it does when the checks are performed synchronously.

@fdummert, is your requirement only for SecurityPolicy.canHandshake() ?

Member

sbordet commented Jul 14, 2016

Thinking about this, until the handshake is complete, the client cannot send any other message.

Having SecurityPolicy.canHandshake() run asynchronously would just move the problem of I/O blocked threads to a different thread, losing in mechanical sympathy for no real benefit - CometD can't process other messages anyway from that client.

Could be different for SecurityPolicy.can[Create|Subscribe|Publish] and forAuthorizer`.
Running those methods asynchronously would allow CometD to process other messages from the same client, but risks to overload the external system, since CometD won't backpressure messages like it does when the checks are performed synchronously.

@fdummert, is your requirement only for SecurityPolicy.canHandshake() ?

@fdummert

This comment has been minimized.

Show comment
Hide comment
@fdummert

fdummert Jul 15, 2016

Hi Simone,

what we are doing in our implementation: we're contacting external systems only during canHandshake and then we're storing enough information in the session to respond to subsequent can[Create|Subscribe|Publish] calls immediately.
If it does not disturb the cometd server to have the canHandshake thread blocked for some time (up to several seconds in worst case, when an external system does not respond in time), then there is no issue. In that case, it might be worth to point out in the docs, that canHandshake indeed may be blocked as an exception to the general rule that advises not to block threads from listeners.

Best Regards,
Florian

Hi Simone,

what we are doing in our implementation: we're contacting external systems only during canHandshake and then we're storing enough information in the session to respond to subsequent can[Create|Subscribe|Publish] calls immediately.
If it does not disturb the cometd server to have the canHandshake thread blocked for some time (up to several seconds in worst case, when an external system does not respond in time), then there is no issue. In that case, it might be worth to point out in the docs, that canHandshake indeed may be blocked as an exception to the general rule that advises not to block threads from listeners.

Best Regards,
Florian

@sbordet

This comment has been minimized.

Show comment
Hide comment
@sbordet

sbordet Jul 15, 2016

Member

It has been correctly pointed out that external systems can have few resources protected by a queue.
For example, 10 DB connections, but a queue with capacity 1000.

If the authorization is synchronous, and 1500 requests arrive at the same time, then 10 requests will hit the DB, 1000 will be queued, causing 1010 threads to block, and 490 requests will be rejected.

With asynchronous authorization, at most 10 threads will be blocked, possibly zero if the DB has an async driver.

Member

sbordet commented Jul 15, 2016

It has been correctly pointed out that external systems can have few resources protected by a queue.
For example, 10 DB connections, but a queue with capacity 1000.

If the authorization is synchronous, and 1500 requests arrive at the same time, then 10 requests will hit the DB, 1000 will be queued, causing 1010 threads to block, and 490 requests will be rejected.

With asynchronous authorization, at most 10 threads will be blocked, possibly zero if the DB has an async driver.

@sbordet sbordet added this to the 4.0.0 milestone May 4, 2017

sbordet added a commit that referenced this issue May 10, 2017

Issue #647 - Async processing.
Updated the API with the newly introduced Promise class.
@sbordet

This comment has been minimized.

Show comment
Hide comment
@sbordet

sbordet Oct 13, 2017

Member

Implemented.

Member

sbordet commented Oct 13, 2017

Implemented.

@sbordet sbordet closed this Oct 13, 2017

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