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

[SR-9876] Adding Data and FlattenSequence of Data is Trimmed #3544

Closed
Mordil opened this issue Feb 7, 2019 · 20 comments
Closed

[SR-9876] Adding Data and FlattenSequence of Data is Trimmed #3544

Mordil opened this issue Feb 7, 2019 · 20 comments

Comments

@Mordil
Copy link

Mordil commented Feb 7, 2019

Previous ID SR-9876
Radar None
Original Reporter @Mordil
Type Bug
Status Closed
Resolution Done

Attachment: Download

Additional Detail from JIRA
Votes 0
Component/s Foundation
Labels Bug
Assignee None
Priority Medium

md5: 1ce37ef1fca9c9fdfb1ed62d2e206bd2

Issue Description:

Using 5.0-DEVELOPMENT-SNAPSHOT-2019-01-16-a the following code works:

extension String {
    func convertedToData() -> Data { return Data(utf8) }
}

func encodeString(...) -> Data

let raw = ["Hello", "World"].map(encodeString).joined()
// expected & actual "$5\r\nHello\r\n$5\r\nWorld\r\n"

let prefix = "*2\r\n".convertedToData()

let result = prefix + raw
// expected "*2\r\n$5\r\nHello\r\n$5\r\nWorld\r\n"
// actual "*2\r\n5\r\nHello\r\n$5\r\nWorld\r\n"

See attached Swift file that reproduces the bug.

Using 5.0-DEVELOPMENT-SNAPSHOT-2019-1-17-a it no longer works.

@belkadan
Copy link

belkadan commented Feb 7, 2019

Is this macOS or Linux? The only difference between those two snapshots on macOS was apple/swift#21929 which is very unlikely to be related.

@belkadan
Copy link

belkadan commented Feb 7, 2019

cc @phausler

@Mordil
Copy link
Author

Mordil commented Feb 7, 2019

@belkadan This is macOS 10.14.4 Beta (18E184e)

@Mordil
Copy link
Author

Mordil commented Feb 7, 2019

If whoever takes this wants to test my exact case that triggers this - it's while running unit tests for NIORedis

The offending code is found in RESPEncoder.swift L34

Just switching between the two toolchains shows the change in output with

case .array(let array):
    let encodedArray = array.map(encode).joined()
    return "*\(array.count)\r\n".convertedToData() + encodedArray

@Mordil
Copy link
Author

Mordil commented Feb 7, 2019

@belkadan @phausler

Using snapshot 1-17 on Ubuntu 18.04 with Docker, this bug does not reproduce.

However, I pushed forward to Snapshot 1-28, and this bug does reproduce.

@phausler
Copy link
Member

swift-DEVELOPMENT-SNAPSHOT-2019-02-03-a-osx works from what I can tell..

```swift
extension String {
func convertedToData() -> Data { return Data(utf8) }
}

func encodeString(_ source: String) -> Data {
let data = source.convertedToData()
return "$(data.count)\r\n".utf8 + data + "\r\n".utf8
}

let raw = ["Hello", "World"].map(encodeString).joined()
do {
let expected = "$5\r\nHello\r\n$5\r\nWorld\r\n"
let actual = String(bytes: raw, encoding: .utf8)!
assert(expected == actual)
}

let prefix = "*2\r\n".convertedToData()

let result = prefix + raw
do {
let expected = "*2\r\n$5\r\nHello\r\n$5\r\nWorld\r\n"
let actual = String(bytes: result, encoding: .utf8)!
assert(expected == actual)
}
```

@phausler
Copy link
Member

swift-5.0-DEVELOPMENT-SNAPSHOT-2019-02-06-a-osx also works

@Mordil
Copy link
Author

Mordil commented Feb 11, 2019

@phausler

Update the code for the line

return "$\(data.count)\r\n".utf8 + data + "\r\n".utf8

so that it's

return "$\(data.count)\r\n".convertedToData() + data + "\r\n".convertedToData()

The original script I wrote was a mistake on my part.

@phausler
Copy link
Member

that also works on those compilers for me on both builds.

@Mordil
Copy link
Author

Mordil commented Feb 11, 2019

This video shows me reproducing on macOS 10.14.4 Beta

SR-9876.mov

@phausler
Copy link
Member

from what I can tell there is no correlation to the host OS of Darwin platforms as long as the toolchain is the same variant. `String(bytes: raw, encoding: .utf8)` does bridge out to objc still.

@Mordil
Copy link
Author

Mordil commented Feb 11, 2019

@phausler

If you are able to pull NIORedis and stand up a Redis instance, then run the unit tests between the two snapshots, you'll see the failure.

I still reliably get the bug using production code.

@phausler
Copy link
Member

I was able to run a 5.0.3 redis server and ran all the RESPEncoder/Decoder tests against the 5.0 2/6 development snapshot.

{{
Test Suite 'Selected tests' started at 2019-02-11 15:10:30.066
Test Suite 'NIORedisTests.xctest' started at 2019-02-11 15:10:30.067
Test Suite 'RESPDecoderByteToMessageDecoderTests' started at 2019-02-11 15:10:30.067
Test Case '-[NIORedisTests.RESPDecoderByteToMessageDecoderTests testDecoding_badMessage_throws]' started.
Test Case '-[NIORedisTests.RESPDecoderByteToMessageDecoderTests testDecoding_badMessage_throws]' passed (0.161 seconds).
Test Case '-[NIORedisTests.RESPDecoderByteToMessageDecoderTests testDecoding_complete_continues]' started.
Test Case '-[NIORedisTests.RESPDecoderByteToMessageDecoderTests testDecoding_complete_continues]' passed (0.003 seconds).
Test Case '-[NIORedisTests.RESPDecoderByteToMessageDecoderTests testDecoding_complete_movesReaderIndex]' started.
Test Case '-[NIORedisTests.RESPDecoderByteToMessageDecoderTests testDecoding_complete_movesReaderIndex]' passed (0.002 seconds).
Test Case '-[NIORedisTests.RESPDecoderByteToMessageDecoderTests testDecoding_partial_needsMoreData]' started.
Test Case '-[NIORedisTests.RESPDecoderByteToMessageDecoderTests testDecoding_partial_needsMoreData]' passed (0.001 seconds).
Test Suite 'RESPDecoderByteToMessageDecoderTests' passed at 2019-02-11 15:10:30.236.
Executed 4 tests, with 0 failures (0 unexpected) in 0.168 (0.170) seconds
Test Suite 'RESPDecoderParsingTests' started at 2019-02-11 15:10:30.237
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_array_handlesMixedTypes]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_array_handlesMixedTypes]' passed (0.001 seconds).
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_array_handlesNestedArrays]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_array_handlesNestedArrays]' passed (0.000 seconds).
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_array_handlesNullElements]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_array_handlesNullElements]' passed (0.001 seconds).
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_array_whenEmpty_returnsEmpty]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_array_whenEmpty_returnsEmpty]' passed (0.001 seconds).
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_array_whenNull_returnsNil]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_array_whenNull_returnsNil]' passed (0.000 seconds).
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_bulkString_handlesMissingEndings]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_bulkString_handlesMissingEndings]' passed (0.001 seconds).
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_bulkString_handlesRawBytes]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_bulkString_handlesRawBytes]' passed (0.001 seconds).
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_bulkString_withNoSize_returnsEmpty]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_bulkString_withNoSize_returnsEmpty]' passed (0.000 seconds).
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_bulkString_withNull_returnsNil]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_bulkString_withNull_returnsNil]' passed (0.000 seconds).
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_bulkString_withSize_returnsContent]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_bulkString_withSize_returnsContent]' passed (0.000 seconds).
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_error]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_error]' passed (0.002 seconds).
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_integer_handlesRecursion]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_integer_handlesRecursion]' passed (0.000 seconds).
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_integer_missingEndings_returnsNil]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_integer_missingEndings_returnsNil]' passed (0.000 seconds).
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_integer_withContent_returnsExpectedContent]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_integer_withContent_returnsExpectedContent]' passed (0.000 seconds).
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_simpleString_handlesRecursion]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_simpleString_handlesRecursion]' passed (0.000 seconds).
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_simpleString_missingEndings_returnsNil]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_simpleString_missingEndings_returnsNil]' passed (0.000 seconds).
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_simpleString_withContent_returnsExpectedContent]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_simpleString_withContent_returnsExpectedContent]' passed (0.001 seconds).
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_simpleString_withNoContent_returnsEmpty]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_simpleString_withNoContent_returnsEmpty]' passed (0.000 seconds).
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_with_arrays]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_with_arrays]' passed (0.000 seconds).
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_with_arrays_multiple]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_with_arrays_multiple]' passed (0.001 seconds).
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_with_bulkString]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_with_bulkString]' passed (0.001 seconds).
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_with_bulkString_multiple]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_with_bulkString_multiple]' passed (0.001 seconds).
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_with_integer]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_with_integer]' passed (0.000 seconds).
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_with_integer_multiple]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_with_integer_multiple]' passed (0.000 seconds).
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_with_simpleString]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_with_simpleString]' passed (0.000 seconds).
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_with_simpleString_multiple]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_with_simpleString_multiple]' passed (0.000 seconds).
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_withInvalidSymbol_throws]' started.
Test Case '-[NIORedisTests.RESPDecoderParsingTests testParsing_withInvalidSymbol_throws]' passed (0.001 seconds).
Test Suite 'RESPDecoderParsingTests' passed at 2019-02-11 15:10:30.293.
Executed 27 tests, with 0 failures (0 unexpected) in 0.015 (0.056) seconds
Test Suite 'RESPDecoderTests' started at 2019-02-11 15:10:30.293
Test Case '-[NIORedisTests.RESPDecoderTests test_all]' started.
Test Case '-[NIORedisTests.RESPDecoderTests test_all]' passed (0.004 seconds).
Test Case '-[NIORedisTests.RESPDecoderTests test_array]' started.
Test Case '-[NIORedisTests.RESPDecoderTests test_array]' passed (0.001 seconds).
Test Case '-[NIORedisTests.RESPDecoderTests test_bulkString]' started.
Test Case '-[NIORedisTests.RESPDecoderTests test_bulkString]' passed (0.003 seconds).
Test Case '-[NIORedisTests.RESPDecoderTests test_error]' started.
Test Case '-[NIORedisTests.RESPDecoderTests test_error]' passed (0.032 seconds).
Test Case '-[NIORedisTests.RESPDecoderTests test_integer]' started.
Test Case '-[NIORedisTests.RESPDecoderTests test_integer]' passed (0.002 seconds).
Test Case '-[NIORedisTests.RESPDecoderTests test_simpleString]' started.
Test Case '-[NIORedisTests.RESPDecoderTests test_simpleString]' passed (0.001 seconds).
Test Suite 'RESPDecoderTests' passed at 2019-02-11 15:10:30.338.
Executed 6 tests, with 0 failures (0 unexpected) in 0.042 (0.044) seconds
Test Suite 'RESPEncoderParsingTests' started at 2019-02-11 15:10:30.338
Test Case '-[NIORedisTests.RESPEncoderParsingTests testArrays]' started.
Test Case '-[NIORedisTests.RESPEncoderParsingTests testArrays]' passed (0.001 seconds).
Test Case '-[NIORedisTests.RESPEncoderParsingTests testBulkStrings]' started.
Test Case '-[NIORedisTests.RESPEncoderParsingTests testBulkStrings]' passed (0.000 seconds).
Test Case '-[NIORedisTests.RESPEncoderParsingTests testError]' started.
Test Case '-[NIORedisTests.RESPEncoderParsingTests testError]' passed (0.003 seconds).
Test Case '-[NIORedisTests.RESPEncoderParsingTests testIntegers]' started.
Test Case '-[NIORedisTests.RESPEncoderParsingTests testIntegers]' passed (0.000 seconds).
Test Case '-[NIORedisTests.RESPEncoderParsingTests testNull]' started.
Test Case '-[NIORedisTests.RESPEncoderParsingTests testNull]' passed (0.001 seconds).
Test Case '-[NIORedisTests.RESPEncoderParsingTests testSimpleStrings]' started.
Test Case '-[NIORedisTests.RESPEncoderParsingTests testSimpleStrings]' passed (0.001 seconds).
Test Suite 'RESPEncoderParsingTests' passed at 2019-02-11 15:10:30.346.
Executed 6 tests, with 0 failures (0 unexpected) in 0.005 (0.008) seconds
Test Suite 'RESPEncoderTests' started at 2019-02-11 15:10:30.346
Test Case '-[NIORedisTests.RESPEncoderTests testArrays]' started.
Test Case '-[NIORedisTests.RESPEncoderTests testArrays]' passed (0.001 seconds).
Test Case '-[NIORedisTests.RESPEncoderTests testBulkStrings]' started.
Test Case '-[NIORedisTests.RESPEncoderTests testBulkStrings]' passed (0.001 seconds).
Test Case '-[NIORedisTests.RESPEncoderTests testError]' started.
Test Case '-[NIORedisTests.RESPEncoderTests testError]' passed (0.001 seconds).
Test Case '-[NIORedisTests.RESPEncoderTests testIntegers]' started.
Test Case '-[NIORedisTests.RESPEncoderTests testIntegers]' passed (0.001 seconds).
Test Case '-[NIORedisTests.RESPEncoderTests testNull]' started.
Test Case '-[NIORedisTests.RESPEncoderTests testNull]' passed (0.000 seconds).
Test Case '-[NIORedisTests.RESPEncoderTests testSimpleStrings]' started.
Test Case '-[NIORedisTests.RESPEncoderTests testSimpleStrings]' passed (0.001 seconds).
Test Suite 'RESPEncoderTests' passed at 2019-02-11 15:10:30.353.
Executed 6 tests, with 0 failures (0 unexpected) in 0.005 (0.007) seconds
Test Suite 'NIORedisTests.xctest' passed at 2019-02-11 15:10:30.354.
Executed 49 tests, with 0 failures (0 unexpected) in 0.236 (0.287) seconds
Test Suite 'Selected tests' passed at 2019-02-11 15:10:30.417.
Executed 49 tests, with 0 failures (0 unexpected) in 0.236 (0.352) seconds
Program ended with exit code: 0
}}

@Mordil
Copy link
Author

Mordil commented Feb 11, 2019

Please run the `BasicCommandsTests` suite, as they have real-use encoding of commands and communication with Redis.

@phausler
Copy link
Member

I am still unable to reproduce the failure:

however here is an alternative code-path:

var data = "*(array.count)\r\n".convertedToData()
for item in array.map(encode) {
data.append(item)
}
return data

that seems a bit clearer to me and does not rely on FlattenSequence being DataProtocol (which I am not sure how that conformance is happening)

@Mordil
Copy link
Author

Mordil commented Feb 11, 2019

Updating to use that process (dropping intermediate usage of FlattenSequence) does resolve the bug.

However, it still concerns me that essentially the first byte of the last element of the FlattenSequence<[Data]> is getting dropped somehow when doing Data() + encodedArray

@Mordil
Copy link
Author

Mordil commented Feb 12, 2019

Also updating the code to be

let encodedArray = array.map(encode).reduce(into: Data(), { $0.append($1) }
return "*\(array.count)\r\n".convertedToData() + encodedArray

works as well.

It seems the issue is when converting FlattenSequence<[Data]> to Data with either Data() + or even Data(encodedArray)

@swift-ci
Copy link
Contributor

Comment by tanner0101 (JIRA)

I'm seeing this issue as well with the following code running in Swift 5 compiler as Swift 4 mode.

https://github.com/vapor/url-encoded-form/blob/master/Sources/URLEncodedForm/Data/URLEncodedFormSerializer.swift#L58

Changing from `joined` to `reduce` fixes serialization:

private extension Array where Element == Data {
    /// Joins an array of `Data` with ampersands.
    func joinedWithAmpersands() -> Data {
        // return Data(self.joined(separator: [.ampersand]))
        return self.reduce(into: Data(), { existing, next in
            existing.append(contentsOf: [.ampersand])
            existing.append(next)
        })
    }
}

I'm using Xcode Version 10.2 beta 3 (10P99q) with 10.2 toolchain selected.
macOS 10.14.2

@swift-ci
Copy link
Contributor

Comment by tanner0101 (JIRA)

This should be reproducible by the following:

@Mordil
Copy link
Author

Mordil commented Mar 28, 2019

As of Swift 5's official release - I have not been able to reproduce this.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@shahmishal shahmishal transferred this issue from apple/swift May 5, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants