Changes to support Edge Server [API] #2202
Merged
+3,408
−4,394
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Jenkins builds are failing due to a CMake error: |
|
Ubuntu in GitHub CI is failing a test due to a runtime exception:
I know how to fix this... |
|
Code Coverage Results:
|
|
Jenkins failures: macOS: The code coverage script push_coverage_results.py blew up somehow. No idea why. Windows: Test "Continuous Super-Fast Push" failed, due to a DatabasePool deadlock: |
|
Windows tests appear to be failing now, and linting is needed |
snej
force-pushed
the
feature/edge-server
branch
from
55b1a07 to
2b5a36b
Compare
snej
force-pushed
the
feature/edge-server
branch
from
ad46321 to
6bd2587
Compare
snej
changed the title
snej
force-pushed
the
feature/edge-server
branch
from
6bd2587 to
219a0c1
Compare
jianminzhao
reviewed
Feb 4, 2025
LiteCore/Support/DatabasePool.cc
Outdated
| logInfo("initial database is %s", nameOf(main).c_str()); | ||
| _dbTag = _c4db_getDatabaseTag(main); | ||
| Cache& cache = writeable() ? _readWrite : _readOnly; | ||
| cache.entries[0].db = std::move(main); |
There was a problem hiding this comment.
What does it mean by moving a pointer?
There was a problem hiding this comment.
That's a no-op -- I should take out the move call. I think it's left over from when it used to be a Retained.
| struct Entry { | ||
| Retained<C4Database> db; ///< Database (may be nullptr) | ||
| unsigned borrowCount = 0; ///< Number of borrows | ||
| std::thread::id borrower; ///< Thread that borrowed it, if any |
There was a problem hiding this comment.
Each thread has one entry that may have multiple borrowers, right? And concurrency occurs among the entries, right?
| if ( message[len - 1] == '\n' ) --len; | ||
| TLSLogDomain.log(kLogLevels[level], "mbedTLS(%s): %.*s", (role == Client ? "C" : "S"), int(len), message); | ||
| // mbedTLS logging callback: | ||
| static const LogLevel kLogLevels[] = {LogLevel::Info, LogLevel::Info, LogLevel::Verbose, LogLevel::Verbose, |
There was a problem hiding this comment.
I saw at some other place this kLogLevels. Can it be moved to a header?
There was a problem hiding this comment.
Despite the name this isn't anything like the others. It maps mbedTLS log levels to LiteCore levels.
| /// Returns the name a database is registered under. | ||
| /// `db` need not be the exact instance that was registered; | ||
| /// any instance on the same database file will work. */ | ||
| std::optional<std::string> nameOfDatabase(C4Database*) const; |
There was a problem hiding this comment.
How can a db have not a name? Assuming db is NONNULL
There was a problem hiding this comment.
It's not the DB's own name, it's the name it's registered as, i.e. the first path component of a URL. If the database isn't being shared, it doesn't have a name.
| static_cast<uint64_t>(c4changes[0].sequence), | ||
| static_cast<uint64_t>(c4changes[nChanges - 1].sequence)); | ||
| _maxSequence = c4changes[nChanges - 1].sequence; | ||
| continue; // ignore changes I made myself |
There was a problem hiding this comment.
Oh! That's a good catch. I thought for a while I had to get rid of that optimization because it wouldn't work with the database pool; but actually it does. I'll put that code back in.
There was a problem hiding this comment.
Actually there was a reason behind that, as I rediscovered when I put it back. That code block is an optimization to ignore "echoes": changes made by a corresponding puller. However it had been nonfunctional for years (the 'external' flag would never be false because it's observing a C4Database that's never used for writes.) With the DatabasePool changes, that code now works, but (a) it exposes a bug somewhere in the pusher that doesn't update the checkpoint, thus the test failure; and (b) it's not going to work correctly in Edge Server where everything is sharing that writeable db.
tl;dr: I took it out again and it fixed the test. I left an explanatory comment in its place.
Returns true if it created or replaced the index, false if it already exists
Just wrapped a try/catch around the entire method so it properly returns the error in `outError` instead of throwing (which would terminate since this is a noexcept method.)
Used by Edge Server, to support the startKey and endKey parameters to `/db/_all_docs`.
Needed by Edge Server so the `/db/_query` handler can validate that the caller provided values for all parameters, and only those params. Also added LIFETIMEBOUND to some methods that return unowned refs.
When called on a background thread they were not logging info about the error.
Changed it from an Assert() to throwMbedTLSError().
- Fixed a bug in existing `split()` when the string ends with the delimiter. - Added new `split()` without a callback, returning a vector. - Added `split2()` that returns a pair. - Added `trimWhitespace`.
SecKeyCreateSignature has a tendency to throw C++ exceptions down inside Apple's code, that are caught and handled therein; this was causing Xcode to break on those 'throws'. Fixed by adding an ExpectingExceptions object around the call.
so that it can be shared between cbl-logtest (in the tools repo) and CppTests
HTTP headers must not contain control characters other than tab, or the delete character. This also catches incorrect linebreaks, like `\n` or `\r`.
This function was moved here from RequestResponse, which is going away
Using a Writer as a backing store was clever but there were some nasty bugs when copying a Headers object. Replaced it with a simpler scheme that just stores a vector of alloc_slices, one per string.
Some fairly normal things would log scary-looking errors that shouldn't appear in Edge Server logs by default!
snej
force-pushed
the
feature/edge-server
branch
from
219a0c1 to
4788ebe
Compare
The old method was groping internal state of the TLSContext, requiring TCPSocket to be a friend of TLSContext. Now TLSContext has a wrapSocket() method that does the dirty work.
- Better error codes: they should be HTTP codes rather than WebSocket (I know it's WebSocketDomain, but it's used for both) - Handle invalid 'Connection' header value - Log errors, usually as warnings except for ECONNRESET since it's pretty common to get it on the server side.
nothing using it yet
These showed up when using DatabasePool, since it vends read-only C4Databases: - C4BlobStore threw a NotFound exception in its constructor if the db didn't already have an Attachments dir - DatabaseImpl::createBlobStore shouldn't try to create the store if read-only, even if `force` is set - When opening a DB, throw if the `create` flag is set but `writeable` isn't; otherwise SQLite will return a confusing SQLITE_MISUSE error - Throw NotWriteable when trying to create an ExclusiveTransaction on a read-only DataFile
snej
force-pushed
the
feature/edge-server
branch
from
4788ebe to
79243a9
Compare
DBAccess now has a DatabasePool instead of using access_lock. If constructed the normal way, it creates its own pool with capacity 2, so it has one read-only and one writeable database as before. It can also be passed an existing DatabasePool, which is what Edge Server does (it has a bigger pool shared by the entire server.) There is plumbing all the way through to c4Replicator, to allow the client to pass in its own DatabasePool. The API changed slightly since it now returns BorrowedDatabase or BorrowedCollection objects. Lots of replicator objects were using C4Collection* pointers even when they didn't have a lock on the DBAccess. This seemed pretty dangerous and didn't play very well with the DatabasePool, so I changed it so each Worker knows its CollectionSpec and uses that.
The implementation of the REST API has moved to Edge Server. C4Listener now only supports the 'Sync' API. - Removed the REST server code, and a few support classes like RequestResponse; that's now in Edge Server. - Modified RESTSyncListener to avoid using the now-missing RequestResponse, which simplified the code. (That's in the EE repo.) C API Changes: - Removed `C4ListenerAPIs` type - Removed `apis` field from `C4ListenerConfig, as well as fields specific to REST APIs, like `directory` - Removed `c4listener_availableAPIs()` - Removed `api` parameter from `c4listener_getURLs()` - Added `serverName` and `serverVersion` fields to `C4ListenerConfig`; you can leave them NULL - Added `c4listener_shareDBWithConfig()` and `C4ListenerDatabaseConfig` struct, for db-specific configuration C++ API Changes: - Removed `C4Listener::availableAPIs()` - Removed `api` parameter from `C4Listener::URLs()` - Added optional `C4ListenerDatabaseConfig const*` parameter to `C4Listener::shareDB()`
The only significant change is "Avoid UBSan error iterating empty SmallVector", which Edge Server was running into.
Almost all of this was done by Jim
snej
force-pushed
the
feature/edge-server
branch
from
79243a9 to
7391250
Compare
jianminzhao
approved these changes
Feb 7, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This branch contains all the LiteCore enhancements & fixes needed to implement Edge Server.
IMPORTANT: There are also changes in the EE repo: https://github.com/couchbase/couchbase-lite-core-EE/pull/47
C API Changes:
C4ListenerAPIstypeapisfield fromC4ListenerConfig, as well as fields specific to REST APIs, likedirectory`c4listener_availableAPIs()apiparameter fromc4listener_getURLs()serverNameandserverVersionfields toC4ListenerConfig; you can leave them NULLc4listener_shareDBWithConfig()andC4ListenerDatabaseConfigstruct, for db-specific configurationC++ API Changes:
C4Listener::availableAPIs()apiparameter fromC4Listener::URLs()C4ListenerDatabaseConfig const*parameter toC4Listener::shareDB()C4Collection::getPurgeCount()C4Database::Transaction::isActive()C4DocEnumeratorconstructor that takes astartKeyparameter.C4Query::parameterNames()[[nodiscard]]and/orLIFETIMEBOUNDattributes to some methods. These will intentionally trigger warnings in buggy code; if so, look carefully and fix your code!