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

Add http transport #449

Merged
merged 7 commits into from
Oct 12, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package com.microsoft.thrifty.transport


import java.io.ByteArrayOutputStream
import java.io.IOException
import java.io.InputStream
import java.net.HttpURLConnection
import java.net.URL

/**
* HTTP implementation of the TTransport interface. Used for working with a
* Thrift web services implementation (using for example TServlet).
*
* THIS IMPLEMENTATION IS NOT THREAD-SAFE !!!
*
* Based on the official thrift java THttpTransport with the apache client support removed.
* Both due to wanting to avoid the additional dependency as well as it being a bit weird to have two
* implementations to switch between in the same class.
*
* Uses HttpURLConnection internally
*
* Also note that under high load, the HttpURLConnection implementation
* may exhaust the open file descriptor limit.
*
* @see [THRIFT-970](https://issues.apache.org/jira/browse/THRIFT-970)
*/
open class THttpTransport(url: String) : Transport {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's petty, but let's drop the T prefix. It's just ugly to me and, is out-of-step with the rest of the project, and (perhaps most importantly) doesn't deliver any actual information.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For context, the CompactProtocol class specifically is heavily based on TCompactProtocol but even there, the prefix had to go. TType retains its prefix only because Type is taken, and "T for Thrift" actually makes sense there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, a substantive comment - can we please make an expect class in src/commonMain/kotlin/com/microsoft/thrifty/transport for this? Then make this one the actual?

Generally on JVM I favor using OkHttp over HttpURLConnection - it's much easier to use correctly and scalably. Is there a reason, aside from the initial implementation using it, to favor HttpURLConnection?

I suppose the fact that it's built in to the JDK is a point in its favor, along with not wanting to bring in extra dependencies. This is part of why I suggested making a separate module for this guy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What benefit do we get from making it an expect/actual class? Why can't there just be different types of Transports implemented for different variants? This is a genuine question, I have no experience with Kotlin Multiplatform.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, main reason is OOB-ness. I don't want to add something to the runtime that drags in a bunch of transitive dependencies. For that I would prefer the separate module. But I see nothing wrong with providing a basic HttpTransport with maybe not the best performance that needs no dependencies and is this simple as part of the "core" library.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fair point re: "OOB-ness". My concern definitely is around the default easy path. Nothing's easier, dependency-wise, than what's built in. On the other hand, this is an Android library first and foremost, and on Android OkHttp is basically the standard anymore. I'm OK with leaving it out for now.

As far as expect/actual, this is the mechanism that lets us have interfaces in common code but have platform-specific implementations. In the eventual iOS release, we'll definitely want to use an HttpTransport. Having an expect class HttpTransport in common code is a prerequisite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to understand the benefit of the expect over a plain interface for this usecase. We can already have different implementations of the http transport, they would all just implement Transport.

So if I understand correctly, expect just tells you "whatever platform this is ported to also needs to implement this thing", which to me is mainly useful for the "common" library to be able to call platform specific code without having the user pass an implementation to some constructor explicitly (there is always going to be exactly one implementation, no more, no less).
To me, plain old interface still sounds totally fitting to me for this use case. Consider the case that we add an OkHttpClient in a separate library as well. Would that then work with the expect? Or would this now be a "plain" implementation of Transport? If yes, why treat both implementations differently?

Please excuse all the text, I am mainly trying to wrap my head around Kotlin Multiplatform.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right that we could just have platform-specific types that implement Transport. The thing I think lacks there is that we won't have a single HttpTransport. That seems like a miss to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I never addressed you question about the benefit(s) of expect class over an interface. The difference is that an expect class is concrete - one can instantiate one as-is, without needing to consider what other type might implement the interface.

private val url: URL = URL(url)
private var currentState: Transport = Writing()
private var connectTimeout: Int? = null
private var readTimeout: Int? = null
private val customHeaders = mutableMapOf<String, String>()
private val sendBuffer = ByteArrayOutputStream()

private inner class Writing : Transport {
override fun read(buffer: ByteArray, offset: Int, count: Int): Int {
error("Currently in writing state")
}

override fun write(buffer: ByteArray, offset: Int, count: Int) {
sendBuffer.write(buffer, offset, count)
}

override fun flush() {
val bytesToSend = sendBuffer.toByteArray()
sendBuffer.reset()
send(bytesToSend)
}

override fun close() {
// do nothing
}
}

private inner class Reading(val inputStream: InputStream) : Transport {
override fun read(buffer: ByteArray, offset: Int, count: Int): Int {
val ret = try {
inputStream.read(buffer, offset, count)
} catch (iox: IOException) {
throw TTransportException(iox)
}
if (ret == -1) {
throw TTransportException("No more data available.")
}
return ret
}

override fun write(buffer: ByteArray, offset: Int, count: Int) {
error("currently in reading state")
}

override fun flush() {
error("currently in reading state")
}

override fun close() {
inputStream.close()
}
}

fun send(data: ByteArray) {
try {
// Create connection object
val connection = url.openConnection() as HttpURLConnection

prepareConnection(connection)
// Make the request
connection.connect()
connection.outputStream.write(data)
val responseCode = connection.responseCode
if (responseCode != HttpURLConnection.HTTP_OK) {
throw TTransportException("HTTP Response code: $responseCode")
}

// Read the response
this.currentState = Reading(connection.inputStream)
} catch (iox: IOException) {
throw TTransportException(iox)
}
}

protected open fun prepareConnection(connection: HttpURLConnection) {
// Timeouts, only if explicitly set
connectTimeout?.let { connection.connectTimeout = it }
readTimeout?.let { connection.readTimeout = it }

connection.requestMethod = "POST"
connection.setRequestProperty("Content-Type", "application/x-thrift")
connection.setRequestProperty("Accept", "application/x-thrift")
connection.setRequestProperty("User-Agent", "Java/THttpClient")
for ((key, value) in customHeaders) {
connection.setRequestProperty(key, value)
}
connection.doOutput = true
}

fun setConnectTimeout(timeout: Int) {
connectTimeout = timeout
}

fun setReadTimeout(timeout: Int) {
readTimeout = timeout
}

fun setCustomHeaders(headers: Map<String, String>) {
customHeaders.clear()
customHeaders.putAll(headers)
}

fun setCustomHeader(key: String, value: String) {
customHeaders[key] = value
}

override fun close() {
currentState.close()
}

override fun read(buffer: ByteArray, offset: Int, count: Int): Int = currentState.read(buffer, offset, count)

override fun write(buffer: ByteArray, offset: Int, count: Int) {
// this mirrors the original behaviour, though it is not very elegant.
// we don't know when the user is done reading, so when they start writing again,
// we just go with it.
if (currentState is Reading) {
currentState.close()
currentState = Writing()
}
currentState.write(buffer, offset, count)
}

override fun flush() {
currentState.flush()
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package com.microsoft.thrifty.transport

class TTransportException : Throwable {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I tend to think this exception doesn't buy us anything, and would prefer not to include it. We could use the perfectly-good ProtocolException in its stead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ProtocolException unfortunately does not have the possibility to pass a cause (doesn't have that constructor implemented), and I really don't like wrapping exceptions in a way that loses information.
But I do agree with you that this does not provide much value, so I will go for just not catching and rethrowing these wrapped.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, SGTM.

constructor(message: String?) : super(message)
constructor(cause: Throwable?) : super(cause)
}