Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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
3 changes: 2 additions & 1 deletion CODEOWNERS
Original file line number Diff line number Diff line change
Expand Up @@ -187,8 +187,9 @@ extensions/filters/http/oauth2 @rgs1 @derekargueta @snowp
/*/extensions/key_value @alyssawilk @ryantheoptimist
# Config Validators
/*/extensions/config/validators/minimum_clusters @adisuissa @htuch
# support library for filesystem based extensions
# File system based extensions
/*/extensions/common/async_files @mattklein123 @ravenblack
/*/extensions/filters/http/file_system_buffer @mattklein123 @ravenblack
# Google Cloud Platform Authentication Filter
/*/extensions/filters/http/gcp_authn @tyxia @yanavlasov
# DNS resolution
Expand Down
18 changes: 18 additions & 0 deletions source/extensions/filters/http/file_system_buffer/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
load(
"//bazel:envoy_build_system.bzl",
"envoy_cc_library",
"envoy_extension_package",
)

licenses(["notice"]) # Apache 2

envoy_extension_package()

envoy_cc_library(
name = "fragment",
srcs = ["fragment.cc"],
hdrs = ["fragment.h"],
deps = [
"//source/extensions/common/async_files",
],
)
134 changes: 134 additions & 0 deletions source/extensions/filters/http/file_system_buffer/fragment.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
#include "source/extensions/filters/http/file_system_buffer/fragment.h"

#include "source/common/buffer/buffer_impl.h"

namespace Envoy {
namespace Extensions {
namespace HttpFilters {
namespace FileSystemBuffer {

class FragmentData {
public:
virtual bool isMemory() const { return false; }
virtual bool isStorage() const { return false; }
virtual ~FragmentData() = default;
};

class MemoryFragment : public FragmentData {
public:
explicit MemoryFragment(Buffer::Instance& buffer);
explicit MemoryFragment(Buffer::Instance& buffer, size_t size);
std::unique_ptr<Buffer::Instance> extract();
bool isMemory() const override { return true; }

private:
std::unique_ptr<Buffer::OwnedImpl> buffer_;
};

class WritingFragment : public FragmentData {};

class ReadingFragment : public FragmentData {};

class StorageFragment : public FragmentData {
public:
explicit StorageFragment(off_t offset) : offset_(offset) {}
off_t offset() const { return offset_; }
bool isStorage() const override { return true; }

private:
const off_t offset_;
};

Fragment::Fragment(Buffer::Instance& buffer)
: size_(buffer.length()), data_(std::make_unique<MemoryFragment>(buffer)) {}

Fragment::Fragment(Buffer::Instance& buffer, size_t size)
: size_(size), data_(std::make_unique<MemoryFragment>(buffer, size)) {}

Fragment::~Fragment() = default;
bool Fragment::isMemory() const { return data_->isMemory(); }
bool Fragment::isStorage() const { return data_->isStorage(); }

MemoryFragment::MemoryFragment(Buffer::Instance& buffer)
: buffer_(std::make_unique<Buffer::OwnedImpl>()) {
buffer_->move(buffer);
}

MemoryFragment::MemoryFragment(Buffer::Instance& buffer, size_t size)
: buffer_(std::make_unique<Buffer::OwnedImpl>()) {
buffer_->move(buffer, size);
}

std::unique_ptr<Buffer::Instance> Fragment::extract() {
ASSERT(isMemory());
auto ret = dynamic_cast<MemoryFragment*>(data_.get())->extract();
data_.reset();
size_ = 0;
return ret;
}

std::unique_ptr<Buffer::Instance> MemoryFragment::extract() { return std::move(buffer_); }

absl::StatusOr<CancelFunction>
Fragment::toStorage(AsyncFileHandle file, off_t offset,
std::function<void(absl::StatusOr<UpdateFragmentFunction>)> on_done) {
ASSERT(isMemory());
auto data = dynamic_cast<MemoryFragment*>(data_.get())->extract();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Instead of using dynamic_cast to deal with the state machine, and potential errors show up as nullptr, can you use absl::Variant instead? Then the state machine and possible values are more clear?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nice suggestion, thanks. Not so keen on more internal implementation details being moved into the header file, which unfortunately is required for variant, but it's better enough to be worth it.

data_ = std::make_unique<WritingFragment>();
return file->write(*data, offset,
[fragment = this, size = size_, offset,
on_done = std::move(on_done)](absl::StatusOr<size_t> result) {
// This lambda always runs in the async file thread, so must not use the
// fragment directly as it may have been destroyed. It is passed through to
// call from on_done's callback, where it can be used again as that function
// is called from the envoy thread only if the filter has *not* been
// destroyed in the meantime.
if (!result.ok()) {
on_done(result.status());
return;
}
if (result.value() != size) {
on_done(absl::AbortedError(fmt::format(
"buffer write wrote {} bytes, wanted {}", result.value(), size)));
return;
}
on_done([fragment, offset]() {
fragment->data_ = std::make_unique<StorageFragment>(offset);
});
});
}

absl::StatusOr<CancelFunction>
Fragment::fromStorage(AsyncFileHandle file,
std::function<void(absl::StatusOr<UpdateFragmentFunction>)> on_done) {
ASSERT(isStorage());
off_t offset = dynamic_cast<StorageFragment*>(data_.get())->offset();
data_ = std::make_unique<ReadingFragment>();
return file->read(
offset, size_,
[fragment = this, size = size_,
on_done](absl::StatusOr<std::unique_ptr<Buffer::Instance>> result) {
// This lambda always runs in the async file thread, so must not use the
// fragment directly as it may have been destroyed. It is passed through to
// call from on_done's callback, where it can be used again as that function
// is called from the envoy thread only if the filter has *not* been destroyed
// in the meantime.
if (!result.ok()) {
on_done(result.status());
return;
}
if (result.value()->length() != size) {
on_done(absl::AbortedError(
fmt::format("buffer read got {} bytes, wanted {}", result.value()->length(), size)));
return;
}
on_done([fragment, data = std::shared_ptr<Buffer::Instance>(result.value().release())]() {
fragment->data_ = std::make_unique<MemoryFragment>(*data);
});
});
}

} // namespace FileSystemBuffer
} // namespace HttpFilters
} // namespace Extensions
} // namespace Envoy
102 changes: 102 additions & 0 deletions source/extensions/filters/http/file_system_buffer/fragment.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
#pragma once
#include <memory>
#include <string>
#include <vector>

#include "envoy/buffer/buffer.h"

#include "source/extensions/common/async_files/async_file_handle.h"

namespace Envoy {
namespace Extensions {
namespace HttpFilters {
namespace FileSystemBuffer {

class FragmentData;
using UpdateFragmentFunction = std::function<void()>;
using Extensions::Common::AsyncFiles::AsyncFileHandle;
using Extensions::Common::AsyncFiles::CancelFunction;

// A Fragment is a piece of the buffer queue used by the filter.
//
// Each fragment may be in memory, on disk, or in an unusable intermediate state
// while controlled by the AsyncFiles library.
//
// The fragments are queued in sequential order, and may not be in the same order
// in the buffer file, as we write "last fragment first" so that the soonest-needed
// fragments remain in memory as long as possible.
//
// Memory fragments are simply `Buffer::Instance`s. Storage fragments contain just
// enough information to reload a memory fragment from the buffer file.
class Fragment {
public:
explicit Fragment(Buffer::Instance& buffer);
explicit Fragment(Buffer::Instance& buffer, size_t size);
~Fragment();

// Starts the transition for this fragment from memory to storage.
//
// The on_done callback is called when the file write completes.
//
// The callback includes its own callback parameter update_fragment (on success),
// so that the operation of updating the fragment can be dispatched to be performed
// in the envoy worker thread, rather than being performed on the callback thread.
// (This is important to allow for proper handling if the context was deleted while
// the operation was in flight, for example.)
// i.e. typical usage resembles:
// auto result = fragment.toStorage(file_handle, offset,
// [this](absl::StatusOr<UpdateFragmentFunction> status_or_update_fragment) {
// if (status_or_update_fragment.ok()) {
// auto update_fragment = status_or_update_fragment.value();
// dispatcher->dispatch([this, update_fragment]() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Where does dispatcher come from in this example? Related to our convo on Slack, this can likely also be gone by the time dispatcher runs. Should some kind of weak_ptr callback system be built into this thing so the caller doesn't need to bother? Or can we at least document more how this is supposed to be used?

Copy link
Copy Markdown
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 keep this helper more context-agnostic, but you're right, it is simpler if the helper gets a dispatch function directly. I still offloaded the safety aspect to the caller, because that way Fragment doesn't need to know anything about filters, but taking a dispatch function parameter makes it much simpler than a callback that returns a callback that must be called back!

// update_fragment();
// // do more stuff with the fragment, or in reaction to it being updated.
// });
// } else {
// dispatcher->dispatch([this, status = status_or_update_fragment.status()]() {
// // do stuff in response to the error.
// });
// }
// });
absl::StatusOr<CancelFunction>
toStorage(AsyncFileHandle file, off_t offset,
std::function<void(absl::StatusOr<UpdateFragmentFunction>)> on_done);

// Starts the transition for this fragment from storage to memory.
//
// The on_done callback is called when the file read completes.
//
// The callback includes its own callback parameter update_fragment (on success),
// so that the operation of updating the fragment can be dispatched to be performed
// in the envoy worker thread, rather than being performed on the callback thread.
// (This is important to allow for proper handling if the context was deleted while
// the operation was in flight, for example.)
//
// See toStorage for example usage.
absl::StatusOr<CancelFunction>
fromStorage(AsyncFileHandle file,
std::function<void(absl::StatusOr<UpdateFragmentFunction>)> on_done);

// Removes the buffer from a memory instance and resets it to size 0.
//
// It is an error to call extract() on a Fragment that is not in memory.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Presumably this returns nullptr? Should this return StatusOr to make precondition failures / programming errors more obvious?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Expanded on the comment to clarify it will cause an exception to call extract on a fragment that is not in memory.

Buffer::InstancePtr extract();

// Returns true if the fragment is in memory, false if in storage or transition.
bool isMemory() const;

// Returns true if the fragment is in storage, false if in memory or transition.
bool isStorage() const;

// Returns the size of the fragment in bytes.
size_t size() const { return size_; }

private:
size_t size_;
std::unique_ptr<FragmentData> data_;
};

} // namespace FileSystemBuffer
} // namespace HttpFilters
} // namespace Extensions
} // namespace Envoy
21 changes: 21 additions & 0 deletions test/extensions/filters/http/file_system_buffer/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
load(
"//bazel:envoy_build_system.bzl",
"envoy_cc_test",
"envoy_package",
)

licenses(["notice"]) # Apache 2

envoy_package()

envoy_cc_test(
name = "fragment_test",
srcs = ["fragment_test.cc"],
tags = ["skip_on_windows"],
deps = [
"//source/extensions/filters/http/file_system_buffer:fragment",
"//test/extensions/common/async_files:mocks",
"//test/mocks/buffer:buffer_mocks",
"//test/test_common:status_utility_lib",
],
)
Loading