Skip to content

binder: BinderServerTransport's shutdown-before-start handling is untested and doesn't actually work #12439

@jdcormie

Description

@jdcormie

BinderServerTransport.java has code like:

public synchronized void start(ServerTransportListener serverTransportListener) {
    this.serverTransportListener = serverTransportListener;
    if (isShutdown()) {
      setState(TransportState.SHUTDOWN_TERMINATED);
      notifyTerminated();
      releaseExecutors();
    } else {

isShutdown() can be true even before start() because the ServerTransportListener we need is provided by ServerListenerImpl.transportCreated() but before that method returns it sets a handshake timer that calls shutdownNow(Status.CANCELLED, ...) on the server transport. BinderServer does call start() as soon as possible so this would be an incredibly difficult race to lose in practice. But unlike ManagedClientTransport, the ServerTransport contract contains no restrictions on when the shutdown methods can be called. So BinderServerTransport's start() method does appear to handle it ...

Except there's no test coverage for the isShutdown() block and it throws IllegalStateException if ever reached. That's because setState() doesn't allow a transition from SHUTDOWN_TERMINATED to itself. This code would also call releaseExecutors() one too many times (it'll already be called by BinderTransport#shutdownInternal)

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions