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

streamr-1.0 fix-start-and-stop-of-autocertifier-client #2182

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/dht/src/connection/ConnectorFacade.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ export class DefaultConnectorFacade implements ConnectorFacade {
this.setLocalPeerDescriptor(localPeerDescriptor)
if (localPeerDescriptor.websocket && !this.config.tlsCertificate && this.config.websocketServerEnableTls) {
try {
await this.websocketConnector.autoCertify()
await this.websocketConnector.startAutocertifierClient()
const connectivityResponse = await this.websocketConnector.checkConnectivity(false)
const autocertifiedLocalPeerDescriptor = this.config.createLocalPeerDescriptor(connectivityResponse)
if (autocertifiedLocalPeerDescriptor.websocket !== undefined) {
Expand Down
27 changes: 16 additions & 11 deletions packages/dht/src/connection/websocket/AutoCertifierClientFacade.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
import {
AutoCertifierClient,
HasSessionRequest,
HasSessionResponse,
HasSessionResponse,
CertifiedSubdomain,
SERVICE_ID as AUTO_CERTIFIER_SERVICE_ID,
HasSession
} from '@streamr/autocertifier-client'
import { ListeningRpcCommunicator } from '../../transport/ListeningRpcCommunicator'
import { Logger, waitForEvent3 } from '@streamr/utils'
import { ITransport } from '../../transport/ITransport'
import { Logger, withTimeout } from '@streamr/utils'
import { ITransport } from '../../transport/ITransport'

const START_TIMEOUT = 60 * 1000

Expand All @@ -20,14 +20,14 @@ const defaultAutoCertifierClientFactory = (
) => new AutoCertifierClient(
configFile,
wsServerPort,
autoCertifierUrl,
autoCertifierUrl,
(_serviceId: string, rpcMethodName: string, method: HasSession) => {
autoCertifierRpcCommunicator.registerRpcMethod(
HasSessionRequest,
HasSessionResponse,
rpcMethodName,
method
)
)
}
)

Expand All @@ -36,7 +36,6 @@ export interface IAutoCertifierClient {
stop(): void
on(eventName: string, cb: (subdomain: CertifiedSubdomain) => void): void
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep this line

interface AutoCertifierClientFacadeConfig {
url: string
configFile: string
Expand All @@ -57,12 +56,13 @@ export class AutoCertifierClientFacade {
private readonly rpcCommunicator: ListeningRpcCommunicator
private readonly setHost: (host: string) => void
private readonly updateCertificate: (certificate: string, privateKey: string) => void
private abortController = new AbortController()

constructor(config: AutoCertifierClientFacadeConfig) {
this.setHost = config.setHost
this.updateCertificate = config.updateCertificate
this.rpcCommunicator = new ListeningRpcCommunicator(AUTO_CERTIFIER_SERVICE_ID, config.transport)
this.autoCertifierClient = config.createClientFactory ? config.createClientFactory()
this.autoCertifierClient = config.createClientFactory ? config.createClientFactory()
: defaultAutoCertifierClientFactory(
config.configFile,
config.url,
Expand All @@ -77,13 +77,18 @@ export class AutoCertifierClientFacade {
this.updateCertificate(subdomain.certificate, subdomain.privateKey)
logger.trace(`Updated certificate`)
})
await Promise.all([
waitForEvent3(this.autoCertifierClient as any, 'updatedCertificate', START_TIMEOUT),
this.autoCertifierClient.start()
])

// waiting for 'updatedCertificate' event is not needed here because
// all paths of this.autoCertifierClient.start() will block until 'updatedCertificate'
// event is emitted or an exception is thrown

await withTimeout(this.autoCertifierClient.start(), START_TIMEOUT,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should separate the starting of the client and obtaining the initial certificate. It doesn't take long time to start the client, but in practice we wait for the certificate?

Also could rename AutoCertifierClientFacade to AutoCertifierFacade as we don't seem to use any of the client's functionality methods (we just call the start and stop methods).

-> maybe in a separate PR

'AutoCertifierClient.start() timed out', this.abortController.signal)

}

stop(): void {
this.abortController.abort()
this.autoCertifierClient.stop()
this.rpcCommunicator.destroy()
}
Expand Down
12 changes: 7 additions & 5 deletions packages/dht/src/connection/websocket/WebsocketConnector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ export class WebsocketConnector {
})
}

public async autoCertify(): Promise<void> {
public async startAutocertifierClient(): Promise<void> {
this.autoCertifierClient = new AutoCertifierClientFacade({
configFile: this.autoCertifierConfigFile,
transport: this.autoCertifierTransport,
Expand All @@ -144,7 +144,7 @@ export class WebsocketConnector {
setHost: (hostName: string) => this.setHost(hostName),
updateCertificate: (certificate: string, privateKey: string) => this.websocketServer!.updateCertificate(certificate, privateKey)
})
logger.trace(`AutoCertifying subdomain...`)
logger.trace(`Starting autocertifier client...`)
await this.autoCertifierClient.start()
}

Expand Down Expand Up @@ -334,10 +334,12 @@ export class WebsocketConnector {

public async destroy(): Promise<void> {
this.abortController.abort()
this.rpcCommunicator.destroy()

const requests = Array.from(this.ongoingConnectRequests.values())
await Promise.allSettled(requests.map((conn) => conn.close(false)))
ptesavol marked this conversation as resolved.
Show resolved Hide resolved
if (this.autoCertifierClient) {
await this.autoCertifierClient.stop()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
await this.autoCertifierClient.stop()
this.autoCertifierClient.stop()

}

this.rpcCommunicator.destroy()

const attempts = Array.from(this.connectingConnections.values())
await Promise.allSettled(attempts.map((conn) => conn.close(false)))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ describe('Failed autocertification', () => {

afterEach(async () => {
await failedAutocertificationNode.stop()
await entryPoint.stop()
await node.stop()
await entryPoint.stop()
})

it('failed auto certification should default to no tls', async () => {
Expand Down
Loading