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.

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

WIP: Initial POC for a new declarative WebSocket server API #38783

Closed
wants to merge 26 commits into from

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Feb 14, 2024

@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file labels Feb 14, 2024
Copy link

quarkus-bot bot commented Feb 14, 2024

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should preferably start with an uppercase character (if it makes sense!)

This message is automatically generated by a bot.

@mkouba
Copy link
Contributor Author

mkouba commented Feb 14, 2024

CC @cescoffier @geoand @brunobat @Ladicek @maxandersen

@mkouba mkouba changed the title [WIP] Initial POC for a new declarative WebSocket server API WIP: Initial POC for a new declarative WebSocket server API Feb 14, 2024
@mkouba
Copy link
Contributor Author

mkouba commented Feb 20, 2024

Note: the current concurrency model needs to be revisited. First we should decide if we limit the concurrency per connection...

@mkouba
Copy link
Contributor Author

mkouba commented Feb 22, 2024

Concurrency model

There are 3 different execution models used to execute an endpoint callback:

  1. Event loop - no parallel processing, ordering not guaranteed
  2. Worked thread - depends on the value of "ordered" argument used for the Context#executeBlocking() method; false - parallel processing is allowed and ordering is not guaranteed; true - ordering is guaranteed, no parallel processing
  3. Virtual thread - always executed on a new thread, the duplicated context is captured, no guarantees wrt parallel processing and ordering

The actual execution model is derived from the method signature and annotations.

The goal is to design and implement a consistent concurrency model that would fit all the execution models listed above.

Vert.x duplicated context

A server connection has an associated Vertx duplicated context. The WebSocketServerConnection is currently stateless. Also @SessionScoped beans should be inherently implemented with concurrent access in mind. However, some frameworks (such as Hibernate Reactive) use the duplicated context to store stateful objects that cannot be used concurrently.

Options

  1. Guarantee that messages are processed serially
  • 👍 no concurrency problems, ordering is well-defined
  • 👎 artificial concurrency limits; processing of one message may block processing of other messages (can be mitigated with timeouts)
  1. Provide no guarantees
  • 👍 parallel processing is allowed, no artifical concurrency limits
  • 👎 no ordering guarantees
  • 👎 frameworks like Hibernate Reactive might be broken
  1. Provide some kind of "partial" guarantee
  • 👍 can be a good compromise
  • 👎 might be confusing for users

Current state

There is a concurrency limiter component that implements option 1. Furthermore, there is a global config property to define the default timeout to process a message. We could add a timeout property to the callback annotations as well.

UPDATE: I've added WebSocket#executionMode() (option 2) according to Ladislav's proposal.

@cescoffier
Copy link
Member

One important thing to mention is that the concurrency is per connection - not global.

That said, I have been rethinking to the dev UI (which uses a web socket). In this case, we can have multiple concurrency messages (on the same connection), and the protocol we use (JSON-RPC) handles the "concurrency" by passing the correlation IDs.

If we do not allow concurrent messages, we might limit ourselves, as there are protocols totally fine with that.

@Ladicek
Copy link
Contributor

Ladicek commented Feb 22, 2024

The nice thing about serial execution is that you get pipelining semantics out of the box (since WebSocket guarantees ordering). That is, a client can send multiple messages, and they will receive responses in the same order. This is nice, and dare I say, should be a default. Being able to opt out of it should be possible, I guess.

@mkouba
Copy link
Contributor Author

mkouba commented Feb 22, 2024

One important thing to mention is that the concurrency is per connection - not global.

@cescoffier Yes, that's a good point. And the concurrency limiter mentioned in the "Current state" is scoped to the endpoint/connection as well.

That said, I have been rethinking to the dev UI (which uses a web socket). In this case, we can have multiple concurrency messages (on the same connection), and the protocol we use (JSON-RPC) handles the "concurrency" by passing the correlation IDs.
If we do not allow concurrent messages, we might limit ourselves, as there are protocols totally fine with that.

I'm not sure I'm following here - the correlation ID is part of the payload, right? So you're saying that ordering does not matter here and possible parallel execution would be a gain.

That is, a client can send multiple messages, and they will receive responses in the same order. This is nice, and dare I say, should be a default. Being able to opt out of it should be possible, I guess.

@Ladicek Hm, a config switch to remove the concurrecy limiter...?

@Ladicek
Copy link
Contributor

Ladicek commented Feb 22, 2024

Not necessarily saying a config switch, that would be extremely coarse grained. I'm not exactly sure what a good granularity would be. The endpoint might work well: @WebSocketEndpoint(path = "/foo", serialExecution = false), something like that.

- also rename WebSocket#value() to WebSocket#path()
- and rename send() methods in test WSClient to sendAndAwait()
- add a test with a session scoped bean
- fire context lifecycle events
- do not associate a new duplicated context with the connection
- create a new duplicated context per message processing
- activate/terminate CDI request context per message processing
- rename DefaultWebSocketEndpoint to WebSocketEndpointBase
- remove the current timeout implementation
- add test for the CDI request context
@mkouba
Copy link
Contributor Author

mkouba commented Feb 29, 2024

I have refactored the contexts (Vert.x, CDI) as follows:

  • we do not associate a single duplicated context with the connection anymore,
  • instead, we create a new duplicated context per message processing (onOpen, onMessage, onClose),
  • therefore we could also activate/terminate a new CDI request context per message processing,
  • we still associate the session context with the connection and active/deactivate the session context per message processing and the session context is destroyed when the connection is closed.

I've kept the default concurrency model (SERIAL).

I've removed the current implementation of timeouts because it was kind of useless. We could address this later (together with error handling).

If there are no objections by next Monday I will squash the commits, rebase on main and mark the PR as ready for review.

@cescoffier @Ladicek @geoand @maxandersen @brunobat

@mkouba
Copy link
Contributor Author

mkouba commented Mar 4, 2024

This pull request is superseded by #39142. However, I will keep this branch to preserve the history...

@mkouba mkouba closed this Mar 4, 2024
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) area/dependencies Pull requests that update a dependency file triage/invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants