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

Async append blocking with timeout #565

Merged
merged 7 commits into from
Jul 14, 2021

Conversation

brenuart
Copy link
Collaborator

@brenuart brenuart commented Jul 8, 2021

Please have a look at this first draft implementation for #559.

In short, the appender supports two distinct operation modes (set via asyncMode):

  • DROP (the default): the appender drops events when the ring buffer is full. This is the legacy behaviour.
  • BLOCK: the appending thread is blocked when the buffer is full until some free space becomes available. An optional timeout can be specified using the retryTimeout configuration property (disabled by default).

The RingBuffer does not allow me to "publish with a timeout" - it offers either a "try publish" or a "publish". The later blocks indefinitely until there is enough space in the buffer to accept the event. Even worst, waiting threads are not unblocked when the disruptor is shutdown.
Instead, I decided to tryPublish and sleep before retrying until the optional timeout expires. Sleeping is done using LockSupport.parkNanos() just like what the Disruptor does too. Retries are scheduled every 100ms which may appear to be too frequent (CPU?). The problem is there is no way I'm aware of that tells me when there is room in the ring buffer. May be I can hook into the EvenHandler and awake any waiting threads when an event has been processed... This means a queue of waiting threads and potential synchronisation issues and/or performance impacts...

Instead of asyncMode and retryTimeout (feel free to comment on the name of these properties) it may be more convenient to collapse them into a single appendTimeout configuration property whose semantic would be:

  • -1: timeout is disabled, means block until space is available (aka the BLOCK mode)
  • 0: times out immediately (aka the DROP mode) - this would be the default behaviour
  • >0: times out after the given time
    When the appender times out, it drops the event and logs a warning as it does already.

@philsttr Feedback?

@brenuart brenuart marked this pull request as draft July 9, 2021 11:24
@brenuart brenuart force-pushed the gh559-async-block branch 3 times, most recently from f15373d to 64901f5 Compare July 10, 2021 10:25
@brenuart
Copy link
Collaborator Author

Sorry for all these force pushes... LogstachTcpAppenderTest#testReconnectOnOpen() was failing on Github but could not reproduce it on my local machine. I had no other choice than experimenting directly on Github :-(

Anyway, this last commit (64901f5) should fix #569 as well.

Calling Disruptor#shutdown(timeout) while the buffer is not empty causes the disruptor to wait in a busy-loop consommuing a lot of CPU. Instead, wait during the grace period before asking the disruptor to shutdown immediately.

Related issue: logfellow#566
Block/drop behaviour is entirely controlled by the value of the “appendTimeout” property:
- `-1` to disable timeout and wait until enough space becomes available
- `0` for no timeout at all and drop the event immediately when the buffer is full
- `> 0` to retry during the specified amount of time
@brenuart
Copy link
Collaborator Author

(rebased on main)

@philsttr
Copy link
Collaborator

Anyway, this last commit (64901f5) should fix #569 as well.

Did you mean #566 ? I don't see how this PR addresses #569. Did I miss something?

@brenuart
Copy link
Collaborator Author

Anyway, this last commit (64901f5) should fix #569 as well.

Did you mean #566 ? I don't see how this PR addresses #569. Did I miss something?

I actually meant #566.
stop() now calls disruptor.halt() which shuts down the disruptor unconditionally and causes a call to LifecycleAware#onShutdown on event handlers implementing that interface - which is the case of the TcpEventHandler. The TCP connection is therefore closed whatever happens.

@philsttr
Copy link
Collaborator

stop() now calls disruptor.halt() which shuts down the disruptor unconditionally and causes a call to LifecycleAware#onShutdown

Got it. Thanks.

@brenuart
Copy link
Collaborator Author

brenuart commented Jul 14, 2021

The PR looks go enough to me now.
@philsttr Tell me what you think... (have a look at the README as well and don't hesitate to rephrase my English)
I'd like to have it merged tho 'cause other changes I have in the pipe rely on it ;-)

@brenuart brenuart marked this pull request as ready for review July 14, 2021 11:31
README.md Outdated Show resolved Hide resolved
@philsttr
Copy link
Collaborator

Looks good! I'll squash and merge after you resolve the existing conflicts.

@brenuart brenuart changed the title First implementation of Async append blocking until an optional timeout Async append blocking with timeout Jul 14, 2021
@philsttr philsttr merged commit c7d9781 into logfellow:main Jul 14, 2021
@philsttr philsttr added this to the 7.0 milestone Jul 14, 2021
@brenuart brenuart deleted the gh559-async-block branch July 14, 2021 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment